diff options
Diffstat (limited to 'crates')
| -rw-r--r-- | crates/fidget-spinner-cli/Cargo.toml | 1 | ||||
| -rw-r--r-- | crates/fidget-spinner-cli/src/mcp/catalog.rs | 44 | ||||
| -rw-r--r-- | crates/fidget-spinner-cli/src/mcp/host/runtime.rs | 235 | ||||
| -rw-r--r-- | crates/fidget-spinner-cli/src/mcp/mod.rs | 1 | ||||
| -rw-r--r-- | crates/fidget-spinner-cli/src/mcp/output.rs | 88 | ||||
| -rw-r--r-- | crates/fidget-spinner-cli/src/mcp/protocol.rs | 11 | ||||
| -rw-r--r-- | crates/fidget-spinner-cli/src/mcp/service.rs | 61 | ||||
| -rw-r--r-- | crates/fidget-spinner-cli/tests/mcp_hardening.rs | 31 |
8 files changed, 324 insertions, 148 deletions
diff --git a/crates/fidget-spinner-cli/Cargo.toml b/crates/fidget-spinner-cli/Cargo.toml index 4ca70e9..51d3cd8 100644 --- a/crates/fidget-spinner-cli/Cargo.toml +++ b/crates/fidget-spinner-cli/Cargo.toml @@ -13,6 +13,7 @@ clap.workspace = true dirs.workspace = true fidget-spinner-core = { path = "../fidget-spinner-core" } fidget-spinner-store-sqlite = { path = "../fidget-spinner-store-sqlite" } +libmcp = { path = "../../../libmcp/crates/libmcp" } serde.workspace = true serde_json.workspace = true time.workspace = true diff --git a/crates/fidget-spinner-cli/src/mcp/catalog.rs b/crates/fidget-spinner-cli/src/mcp/catalog.rs index 178b980..ec57a5c 100644 --- a/crates/fidget-spinner-cli/src/mcp/catalog.rs +++ b/crates/fidget-spinner-cli/src/mcp/catalog.rs @@ -1,5 +1,8 @@ +use libmcp::ReplayContract; use serde_json::{Value, json}; +use crate::mcp::output::with_render_property; + #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub(crate) enum DispatchTarget { Host, @@ -7,12 +10,6 @@ pub(crate) enum DispatchTarget { } #[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub(crate) enum ReplayContract { - SafeReplay, - NeverReplay, -} - -#[derive(Clone, Copy, Debug, Eq, PartialEq)] pub(crate) struct ToolSpec { pub name: &'static str, pub description: &'static str, @@ -32,7 +29,7 @@ impl ToolSpec { pub fn annotation_json(self) -> Value { json!({ "title": self.name, - "readOnlyHint": self.replay == ReplayContract::SafeReplay, + "readOnlyHint": self.replay == ReplayContract::Convergent, "destructiveHint": self.replay == ReplayContract::NeverReplay, "fidgetSpinner": { "dispatch": match self.dispatch { @@ -40,7 +37,8 @@ impl ToolSpec { DispatchTarget::Worker => "worker", }, "replayContract": match self.replay { - ReplayContract::SafeReplay => "safe_replay", + ReplayContract::Convergent => "convergent", + ReplayContract::ProbeRequired => "probe_required", ReplayContract::NeverReplay => "never_replay", }, } @@ -61,25 +59,25 @@ pub(crate) fn tool_spec(name: &str) -> Option<ToolSpec> { name: "project.status", description: "Read local project status, store paths, and git availability for the currently bound project.", dispatch: DispatchTarget::Worker, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), "project.schema" => Some(ToolSpec { name: "project.schema", description: "Read the project-local payload schema and field validation tiers.", dispatch: DispatchTarget::Worker, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), "frontier.list" => Some(ToolSpec { name: "frontier.list", description: "List frontiers for the current project.", dispatch: DispatchTarget::Worker, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), "frontier.status" => Some(ToolSpec { name: "frontier.status", description: "Read one frontier projection, including champion and active candidates.", dispatch: DispatchTarget::Worker, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), "frontier.init" => Some(ToolSpec { name: "frontier.init", @@ -103,13 +101,13 @@ pub(crate) fn tool_spec(name: &str) -> Option<ToolSpec> { name: "node.list", description: "List recent nodes. Archived nodes are hidden unless explicitly requested.", dispatch: DispatchTarget::Worker, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), "node.read" => Some(ToolSpec { name: "node.read", description: "Read one node including payload, diagnostics, and hidden annotations.", dispatch: DispatchTarget::Worker, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), "node.annotate" => Some(ToolSpec { name: "node.annotate", @@ -145,25 +143,25 @@ pub(crate) fn tool_spec(name: &str) -> Option<ToolSpec> { name: "skill.list", description: "List bundled skills shipped with this package.", dispatch: DispatchTarget::Host, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), "skill.show" => Some(ToolSpec { name: "skill.show", description: "Return one bundled skill text shipped with this package. Defaults to `fidget-spinner` when name is omitted.", dispatch: DispatchTarget::Host, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), "system.health" => Some(ToolSpec { name: "system.health", description: "Read MCP host health, session binding, worker generation, rollout state, and the last fault.", dispatch: DispatchTarget::Host, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), "system.telemetry" => Some(ToolSpec { name: "system.telemetry", description: "Read aggregate request, retry, restart, and per-operation telemetry for this MCP session.", dispatch: DispatchTarget::Host, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), _ => None, } @@ -175,22 +173,22 @@ pub(crate) fn resource_spec(uri: &str) -> Option<ResourceSpec> { "fidget-spinner://project/config" => Some(ResourceSpec { uri: "fidget-spinner://project/config", dispatch: DispatchTarget::Worker, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), "fidget-spinner://project/schema" => Some(ResourceSpec { uri: "fidget-spinner://project/schema", dispatch: DispatchTarget::Worker, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), "fidget-spinner://skill/fidget-spinner" => Some(ResourceSpec { uri: "fidget-spinner://skill/fidget-spinner", dispatch: DispatchTarget::Host, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), "fidget-spinner://skill/frontier-loop" => Some(ResourceSpec { uri: "fidget-spinner://skill/frontier-loop", dispatch: DispatchTarget::Host, - replay: ReplayContract::SafeReplay, + replay: ReplayContract::Convergent, }), _ => None, } @@ -225,7 +223,7 @@ pub(crate) fn tool_definitions() -> Vec<Value> { json!({ "name": spec.name, "description": spec.description, - "inputSchema": input_schema(spec.name), + "inputSchema": with_render_property(input_schema(spec.name)), "annotations": spec.annotation_json(), }) }) diff --git a/crates/fidget-spinner-cli/src/mcp/host/runtime.rs b/crates/fidget-spinner-cli/src/mcp/host/runtime.rs index dd75544..17c26c7 100644 --- a/crates/fidget-spinner-cli/src/mcp/host/runtime.rs +++ b/crates/fidget-spinner-cli/src/mcp/host/runtime.rs @@ -5,6 +5,10 @@ use std::path::PathBuf; use std::process::Command; use std::time::Instant; +use libmcp::{ + FramedMessage, HostSessionKernel, ReplayContract, RequestId, load_snapshot_file_from_env, + remove_snapshot_file, write_snapshot_file, +}; use serde::Serialize; use serde_json::{Value, json}; @@ -14,13 +18,13 @@ use super::{ process::{ProjectBinding, WorkerSupervisor}, }; use crate::mcp::catalog::{ - DispatchTarget, ReplayContract, list_resources, resource_spec, tool_definitions, tool_spec, + DispatchTarget, list_resources, resource_spec, tool_definitions, tool_spec, }; use crate::mcp::fault::{FaultKind, FaultRecord, FaultStage}; +use crate::mcp::output::split_render_mode; use crate::mcp::protocol::{ CRASH_ONCE_ENV, FORCE_ROLLOUT_ENV, HOST_STATE_ENV, HostRequestId, HostStateSeed, - PROTOCOL_VERSION, ProjectBindingSeed, SERVER_NAME, SessionSeed, WorkerOperation, - WorkerSpawnConfig, + PROTOCOL_VERSION, ProjectBindingSeed, SERVER_NAME, WorkerOperation, WorkerSpawnConfig, }; use crate::mcp::telemetry::{ BinaryHealth, BindingHealth, HealthSnapshot, InitializationHealth, ServerTelemetry, @@ -59,7 +63,7 @@ pub(crate) fn run_host( struct HostRuntime { config: HostConfig, binding: Option<ProjectBinding>, - session: SessionSeed, + session_kernel: HostSessionKernel, telemetry: ServerTelemetry, next_request_id: u64, worker: WorkerSupervisor, @@ -73,10 +77,13 @@ struct HostRuntime { impl HostRuntime { fn new(config: HostConfig) -> Result<Self, fidget_spinner_store_sqlite::StoreError> { - let restored = restore_host_state(); - let session = restored + let restored = restore_host_state()?; + let session_kernel = restored .as_ref() - .map_or_else(SessionSeed::default, |seed| seed.session.clone()); + .map(|seed| seed.session_kernel.clone().restore()) + .transpose() + .map_err(fidget_spinner_store_sqlite::StoreError::Io)? + .map_or_else(HostSessionKernel::cold, HostSessionKernel::from_restored); let telemetry = restored .as_ref() .map_or_else(ServerTelemetry::default, |seed| seed.telemetry.clone()); @@ -117,7 +124,7 @@ impl HostRuntime { Ok(Self { config: config.clone(), binding, - session, + session_kernel, telemetry, next_request_id, worker, @@ -131,8 +138,8 @@ impl HostRuntime { } fn handle_line(&mut self, line: &str) -> Option<Value> { - let message = match serde_json::from_str::<Value>(line) { - Ok(message) => message, + let frame = match FramedMessage::parse(line.as_bytes().to_vec()) { + Ok(frame) => frame, Err(error) => { return Some(jsonrpc_error( Value::Null, @@ -145,11 +152,12 @@ impl HostRuntime { )); } }; - self.handle_message(message) + self.handle_frame(frame) } - fn handle_message(&mut self, message: Value) -> Option<Value> { - let Some(object) = message.as_object() else { + fn handle_frame(&mut self, frame: FramedMessage) -> Option<Value> { + self.session_kernel.observe_client_frame(&frame); + let Some(object) = frame.value.as_object() else { return Some(jsonrpc_error( Value::Null, FaultRecord::new( @@ -168,7 +176,7 @@ impl HostRuntime { let started_at = Instant::now(); self.telemetry.record_request(&operation_key); - let response = match self.dispatch(method, params, id.clone()) { + let response = match self.dispatch(&frame, method, params, id.clone()) { Ok(Some(result)) => { self.telemetry .record_success(&operation_key, started_at.elapsed().as_millis()); @@ -206,29 +214,26 @@ impl HostRuntime { fn dispatch( &mut self, + request_frame: &FramedMessage, method: &str, params: Value, request_id: Option<Value>, ) -> Result<Option<Value>, FaultRecord> { match method { - "initialize" => { - self.session.initialize_params = Some(params.clone()); - self.session.initialized = false; - Ok(Some(json!({ - "protocolVersion": PROTOCOL_VERSION, - "capabilities": { - "tools": { "listChanged": false }, - "resources": { "listChanged": false, "subscribe": false } - }, - "serverInfo": { - "name": SERVER_NAME, - "version": env!("CARGO_PKG_VERSION") - }, - "instructions": "The DAG is canonical truth. Frontier state is derived. Bind the session with project.bind before project-local DAG operations when the MCP is running unbound." - }))) - } + "initialize" => Ok(Some(json!({ + "protocolVersion": PROTOCOL_VERSION, + "capabilities": { + "tools": { "listChanged": false }, + "resources": { "listChanged": false, "subscribe": false } + }, + "serverInfo": { + "name": SERVER_NAME, + "version": env!("CARGO_PKG_VERSION") + }, + "instructions": "The DAG is canonical truth. Frontier state is derived. Bind the session with project.bind before project-local DAG operations when the MCP is running unbound." + }))), "notifications/initialized" => { - if self.session.initialize_params.is_none() { + if !self.seed_captured() { return Err(FaultRecord::new( FaultKind::NotInitialized, FaultStage::Host, @@ -236,7 +241,6 @@ impl HostRuntime { "received initialized notification before initialize", )); } - self.session.initialized = true; Ok(None) } "notifications/cancelled" => Ok(None), @@ -246,8 +250,14 @@ impl HostRuntime { match other { "tools/list" => Ok(Some(json!({ "tools": tool_definitions() }))), "resources/list" => Ok(Some(json!({ "resources": list_resources() }))), - "tools/call" => Ok(Some(self.dispatch_tool_call(params, request_id)?)), - "resources/read" => Ok(Some(self.dispatch_resource_read(params)?)), + "tools/call" => Ok(Some(self.dispatch_tool_call( + request_frame, + params, + request_id, + )?)), + "resources/read" => { + Ok(Some(self.dispatch_resource_read(request_frame, params)?)) + } _ => Err(FaultRecord::new( FaultKind::InvalidInput, FaultStage::Protocol, @@ -261,6 +271,7 @@ impl HostRuntime { fn dispatch_tool_call( &mut self, + request_frame: &FramedMessage, params: Value, _request_id: Option<Value>, ) -> Result<Value, FaultRecord> { @@ -275,11 +286,17 @@ impl HostRuntime { })?; match spec.dispatch { DispatchTarget::Host => self.handle_host_tool(&envelope.name, envelope.arguments), - DispatchTarget::Worker => self.dispatch_worker_tool(spec, envelope.arguments), + DispatchTarget::Worker => { + self.dispatch_worker_tool(request_frame, spec, envelope.arguments) + } } } - fn dispatch_resource_read(&mut self, params: Value) -> Result<Value, FaultRecord> { + fn dispatch_resource_read( + &mut self, + request_frame: &FramedMessage, + params: Value, + ) -> Result<Value, FaultRecord> { let args = deserialize::<ReadResourceArgs>(params, "resources/read")?; let spec = resource_spec(&args.uri).ok_or_else(|| { FaultRecord::new( @@ -292,6 +309,7 @@ impl HostRuntime { match spec.dispatch { DispatchTarget::Host => Ok(Self::handle_host_resource(spec.uri)), DispatchTarget::Worker => self.dispatch_worker_operation( + request_frame, format!("resources/read:{}", args.uri), spec.replay, WorkerOperation::ReadResource { uri: args.uri }, @@ -301,11 +319,13 @@ impl HostRuntime { fn dispatch_worker_tool( &mut self, + request_frame: &FramedMessage, spec: crate::mcp::catalog::ToolSpec, arguments: Value, ) -> Result<Value, FaultRecord> { let operation = format!("tools/call:{}", spec.name); self.dispatch_worker_operation( + request_frame, operation.clone(), spec.replay, WorkerOperation::CallTool { @@ -317,6 +337,7 @@ impl HostRuntime { fn dispatch_worker_operation( &mut self, + request_frame: &FramedMessage, operation: String, replay: ReplayContract, worker_operation: WorkerOperation, @@ -328,21 +349,34 @@ impl HostRuntime { self.worker.arm_crash_once(); } + self.session_kernel + .record_forwarded_request(request_frame, replay); + let forwarded_request_id = request_id_from_frame(request_frame); let request_id = self.allocate_request_id(); match self.worker.execute(request_id, worker_operation.clone()) { - Ok(result) => Ok(result), + Ok(result) => { + self.complete_forwarded_request(forwarded_request_id.as_ref()); + Ok(result) + } Err(fault) => { - if replay == ReplayContract::SafeReplay && fault.retryable { + if replay == ReplayContract::Convergent && fault.retryable { self.telemetry.record_retry(&operation); self.telemetry.record_worker_restart(); self.worker .restart() .map_err(|restart_fault| restart_fault.mark_retried())?; match self.worker.execute(request_id, worker_operation) { - Ok(result) => Ok(result), - Err(retry_fault) => Err(retry_fault.mark_retried()), + Ok(result) => { + self.complete_forwarded_request(forwarded_request_id.as_ref()); + Ok(result) + } + Err(retry_fault) => { + self.complete_forwarded_request(forwarded_request_id.as_ref()); + Err(retry_fault.mark_retried()) + } } } else { + self.complete_forwarded_request(forwarded_request_id.as_ref()); Err(fault) } } @@ -350,6 +384,8 @@ impl HostRuntime { } fn handle_host_tool(&mut self, name: &str, arguments: Value) -> Result<Value, FaultRecord> { + let operation = format!("tools/call:{name}"); + let (render, arguments) = split_render_mode(arguments, &operation, FaultStage::Host)?; match name { "project.bind" => { let args = deserialize::<ProjectBindArgs>(arguments, "tools/call:project.bind")?; @@ -357,11 +393,14 @@ impl HostRuntime { .map_err(host_store_fault("tools/call:project.bind"))?; self.worker.rebind(resolved.binding.project_root.clone()); self.binding = Some(resolved.binding); - tool_success(&resolved.status) + tool_success(&resolved.status, render) } - "skill.list" => tool_success(&json!({ - "skills": crate::bundled_skill::bundled_skill_summaries(), - })), + "skill.list" => tool_success( + &json!({ + "skills": crate::bundled_skill::bundled_skill_summaries(), + }), + render, + ), "skill.show" => { let args = deserialize::<SkillShowArgs>(arguments, "tools/call:skill.show")?; let skill = args.name.as_deref().map_or_else( @@ -377,31 +416,37 @@ impl HostRuntime { }) }, )?; - tool_success(&json!({ - "name": skill.name, - "description": skill.description, - "resource_uri": skill.resource_uri, - "body": skill.body, - })) + tool_success( + &json!({ + "name": skill.name, + "description": skill.description, + "resource_uri": skill.resource_uri, + "body": skill.body, + }), + render, + ) } - "system.health" => tool_success(&HealthSnapshot { - initialization: InitializationHealth { - ready: self.session.initialized, - seed_captured: self.session.initialize_params.is_some(), - }, - binding: binding_health(self.binding.as_ref()), - worker: WorkerHealth { - worker_generation: self.worker.generation(), - alive: self.worker.is_alive(), - }, - binary: BinaryHealth { - current_executable: self.binary.path.display().to_string(), - launch_path_stable: self.binary.launch_path_stable, - rollout_pending: self.binary.rollout_pending().unwrap_or(false), + "system.health" => tool_success( + &HealthSnapshot { + initialization: InitializationHealth { + ready: self.session_initialized(), + seed_captured: self.seed_captured(), + }, + binding: binding_health(self.binding.as_ref()), + worker: WorkerHealth { + worker_generation: self.worker.generation(), + alive: self.worker.is_alive(), + }, + binary: BinaryHealth { + current_executable: self.binary.path.display().to_string(), + launch_path_stable: self.binary.launch_path_stable, + rollout_pending: self.binary.rollout_pending().unwrap_or(false), + }, + last_fault: self.telemetry.last_fault.clone(), }, - last_fault: self.telemetry.last_fault.clone(), - }), - "system.telemetry" => tool_success(&self.telemetry), + render, + ), + "system.telemetry" => tool_success(&self.telemetry, render), other => Err(FaultRecord::new( FaultKind::InvalidInput, FaultStage::Host, @@ -425,7 +470,7 @@ impl HostRuntime { } fn require_initialized(&self, operation: &str) -> Result<(), FaultRecord> { - if self.session.initialized { + if self.session_initialized() { return Ok(()); } Err(FaultRecord::new( @@ -447,6 +492,22 @@ impl HostRuntime { }) } + fn session_initialized(&self) -> bool { + self.session_kernel + .initialization_seed() + .is_some_and(|seed| seed.initialized_notification.is_some()) + } + + fn seed_captured(&self) -> bool { + self.session_kernel.initialization_seed().is_some() + } + + fn complete_forwarded_request(&mut self, request_id: Option<&RequestId>) { + if let Some(request_id) = request_id { + let _ = self.session_kernel.take_completed_request(request_id); + } + } + fn allocate_request_id(&mut self) -> HostRequestId { let id = HostRequestId(self.next_request_id); self.next_request_id += 1; @@ -466,7 +527,7 @@ impl HostRuntime { fn roll_forward(&mut self) -> Result<(), fidget_spinner_store_sqlite::StoreError> { let state = HostStateSeed { - session: self.session.clone(), + session_kernel: self.session_kernel.snapshot(), telemetry: self.telemetry.clone(), next_request_id: self.next_request_id, binding: self.binding.clone().map(ProjectBindingSeed::from), @@ -474,20 +535,23 @@ impl HostRuntime { force_rollout_consumed: self.force_rollout_consumed, crash_once_consumed: self.crash_once_consumed, }; - let serialized = serde_json::to_string(&state)?; + let state_path = write_snapshot_file("fidget-spinner-mcp-host-reexec", &state) + .map_err(fidget_spinner_store_sqlite::StoreError::Io)?; let mut command = Command::new(&self.binary.path); let _ = command.arg("mcp").arg("serve"); if let Some(project) = self.config.initial_project.as_ref() { let _ = command.arg("--project").arg(project); } - let _ = command.env(HOST_STATE_ENV, serialized); + let _ = command.env(HOST_STATE_ENV, &state_path); #[cfg(unix)] { let error = command.exec(); + let _removed = remove_snapshot_file(&state_path); Err(fidget_spinner_store_sqlite::StoreError::Io(error)) } #[cfg(not(unix))] { + let _removed = remove_snapshot_file(&state_path); return Err(fidget_spinner_store_sqlite::StoreError::Io(io::Error::new( io::ErrorKind::Unsupported, "host rollout requires unix exec support", @@ -605,9 +669,8 @@ impl From<ProjectBinding> for ProjectBindingSeed { } } -fn restore_host_state() -> Option<HostStateSeed> { - let raw = std::env::var(HOST_STATE_ENV).ok()?; - serde_json::from_str::<HostStateSeed>(&raw).ok() +fn restore_host_state() -> Result<Option<HostStateSeed>, fidget_spinner_store_sqlite::StoreError> { + load_snapshot_file_from_env(HOST_STATE_ENV).map_err(fidget_spinner_store_sqlite::StoreError::Io) } fn deserialize<T: for<'de> serde::Deserialize<'de>>( @@ -638,19 +701,17 @@ fn operation_key(method: &str, params: &Value) -> String { } } -fn tool_success(value: &impl Serialize) -> Result<Value, FaultRecord> { - Ok(json!({ - "content": [{ - "type": "text", - "text": crate::to_pretty_json(value).map_err(|error| { - FaultRecord::new(FaultKind::Internal, FaultStage::Host, "tool_success", error.to_string()) - })?, - }], - "structuredContent": serde_json::to_value(value).map_err(|error| { - FaultRecord::new(FaultKind::Internal, FaultStage::Host, "tool_success", error.to_string()) - })?, - "isError": false, - })) +fn request_id_from_frame(frame: &FramedMessage) -> Option<RequestId> { + match frame.classify() { + libmcp::RpcEnvelopeKind::Request { id, .. } => Some(id), + libmcp::RpcEnvelopeKind::Notification { .. } + | libmcp::RpcEnvelopeKind::Response { .. } + | libmcp::RpcEnvelopeKind::Unknown => None, + } +} + +fn tool_success(value: &impl Serialize, render: libmcp::RenderMode) -> Result<Value, FaultRecord> { + crate::mcp::output::tool_success(value, render, FaultStage::Host, "tool_success") } fn host_store_fault( diff --git a/crates/fidget-spinner-cli/src/mcp/mod.rs b/crates/fidget-spinner-cli/src/mcp/mod.rs index adea066..d219e96 100644 --- a/crates/fidget-spinner-cli/src/mcp/mod.rs +++ b/crates/fidget-spinner-cli/src/mcp/mod.rs @@ -1,6 +1,7 @@ mod catalog; mod fault; mod host; +mod output; mod protocol; mod service; mod telemetry; diff --git a/crates/fidget-spinner-cli/src/mcp/output.rs b/crates/fidget-spinner-cli/src/mcp/output.rs new file mode 100644 index 0000000..58f7eb4 --- /dev/null +++ b/crates/fidget-spinner-cli/src/mcp/output.rs @@ -0,0 +1,88 @@ +use libmcp::{JsonPorcelainConfig, RenderMode, render_json_porcelain}; +use serde::Serialize; +use serde_json::{Map, Value, json}; + +use crate::mcp::fault::{FaultKind, FaultRecord, FaultStage}; + +pub(crate) fn split_render_mode( + arguments: Value, + operation: &str, + stage: FaultStage, +) -> Result<(RenderMode, Value), FaultRecord> { + let Value::Object(mut object) = arguments else { + return Ok((RenderMode::Porcelain, arguments)); + }; + let render = object + .remove("render") + .map(|value| { + serde_json::from_value::<RenderMode>(value).map_err(|error| { + FaultRecord::new( + FaultKind::InvalidInput, + stage, + operation, + format!("invalid render mode: {error}"), + ) + }) + }) + .transpose()? + .unwrap_or(RenderMode::Porcelain); + Ok((render, Value::Object(object))) +} + +pub(crate) fn tool_success( + value: &impl Serialize, + render: RenderMode, + stage: FaultStage, + operation: &str, +) -> Result<Value, FaultRecord> { + let structured = serde_json::to_value(value).map_err(|error| { + FaultRecord::new(FaultKind::Internal, stage, operation, error.to_string()) + })?; + tool_success_from_value(structured, render, stage, operation) +} + +pub(crate) fn tool_success_from_value( + structured: Value, + render: RenderMode, + stage: FaultStage, + operation: &str, +) -> Result<Value, FaultRecord> { + let text = match render { + RenderMode::Porcelain => render_json_porcelain(&structured, JsonPorcelainConfig::default()), + RenderMode::Json => crate::to_pretty_json(&structured).map_err(|error| { + FaultRecord::new(FaultKind::Internal, stage, operation, error.to_string()) + })?, + }; + Ok(json!({ + "content": [{ + "type": "text", + "text": text, + }], + "structuredContent": structured, + "isError": false, + })) +} + +pub(crate) fn with_render_property(schema: Value) -> Value { + let Value::Object(mut object) = schema else { + return schema; + }; + + let properties = object + .entry("properties".to_owned()) + .or_insert_with(|| Value::Object(Map::new())); + if let Value::Object(properties) = properties { + let _ = properties.insert( + "render".to_owned(), + json!({ + "type": "string", + "enum": ["porcelain", "json"], + "description": "Output mode. Defaults to porcelain for model-friendly summaries." + }), + ); + } + let _ = object + .entry("additionalProperties".to_owned()) + .or_insert(Value::Bool(false)); + Value::Object(object) +} diff --git a/crates/fidget-spinner-cli/src/mcp/protocol.rs b/crates/fidget-spinner-cli/src/mcp/protocol.rs index 1f24f37..f48d881 100644 --- a/crates/fidget-spinner-cli/src/mcp/protocol.rs +++ b/crates/fidget-spinner-cli/src/mcp/protocol.rs @@ -1,5 +1,6 @@ use std::path::PathBuf; +use libmcp::HostSessionKernelSnapshot; use serde::{Deserialize, Serialize}; use serde_json::Value; @@ -14,15 +15,9 @@ pub(crate) const TRANSIENT_ONCE_ENV: &str = "FIDGET_SPINNER_MCP_TEST_WORKER_TRAN pub(crate) const TRANSIENT_ONCE_MARKER_ENV: &str = "FIDGET_SPINNER_MCP_TEST_WORKER_TRANSIENT_ONCE_MARKER"; -#[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq, Serialize)] -pub(crate) struct SessionSeed { - pub initialize_params: Option<Value>, - pub initialized: bool, -} - -#[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] pub(crate) struct HostStateSeed { - pub session: SessionSeed, + pub session_kernel: HostSessionKernelSnapshot, pub telemetry: ServerTelemetry, pub next_request_id: u64, pub binding: Option<ProjectBindingSeed>, diff --git a/crates/fidget-spinner-cli/src/mcp/service.rs b/crates/fidget-spinner-cli/src/mcp/service.rs index a7cae10..6b9c5da 100644 --- a/crates/fidget-spinner-cli/src/mcp/service.rs +++ b/crates/fidget-spinner-cli/src/mcp/service.rs @@ -11,10 +11,12 @@ use fidget_spinner_store_sqlite::{ CloseExperimentRequest, CreateFrontierRequest, CreateNodeRequest, EdgeAttachment, EdgeAttachmentDirection, ListNodesQuery, ProjectStore, StoreError, }; +use libmcp::RenderMode; use serde::Deserialize; use serde_json::{Map, Value, json}; use crate::mcp::fault::{FaultKind, FaultRecord, FaultStage}; +use crate::mcp::output::split_render_mode; use crate::mcp::protocol::{TRANSIENT_ONCE_ENV, TRANSIENT_ONCE_MARKER_ENV, WorkerOperation}; pub(crate) struct WorkerService { @@ -42,22 +44,28 @@ impl WorkerService { } fn call_tool(&mut self, name: &str, arguments: Value) -> Result<Value, FaultRecord> { + let operation = format!("tools/call:{name}"); + let (render, arguments) = split_render_mode(arguments, &operation, FaultStage::Worker)?; match name { - "project.status" => tool_success(&json!({ - "project_root": self.store.project_root(), - "state_root": self.store.state_root(), - "display_name": self.store.config().display_name, - "schema": self.store.schema().schema_ref(), - "git_repo_detected": crate::run_git(self.store.project_root(), &["rev-parse", "--show-toplevel"]) - .map_err(store_fault("tools/call:project.status"))? - .is_some(), - })), - "project.schema" => tool_success(self.store.schema()), + "project.status" => tool_success( + &json!({ + "project_root": self.store.project_root(), + "state_root": self.store.state_root(), + "display_name": self.store.config().display_name, + "schema": self.store.schema().schema_ref(), + "git_repo_detected": crate::run_git(self.store.project_root(), &["rev-parse", "--show-toplevel"]) + .map_err(store_fault("tools/call:project.status"))? + .is_some(), + }), + render, + ), + "project.schema" => tool_success(self.store.schema(), render), "frontier.list" => tool_success( &self .store .list_frontiers() .map_err(store_fault("tools/call:frontier.list"))?, + render, ), "frontier.status" => { let args = deserialize::<FrontierStatusToolArgs>(arguments)?; @@ -69,6 +77,7 @@ impl WorkerService { .map_err(store_fault("tools/call:frontier.status"))?, ) .map_err(store_fault("tools/call:frontier.status"))?, + render, ) } "frontier.init" => { @@ -124,7 +133,7 @@ impl WorkerService { initial_checkpoint, }) .map_err(store_fault("tools/call:frontier.init"))?; - tool_success(&projection) + tool_success(&projection, render) } "node.create" => { let args = deserialize::<NodeCreateToolArgs>(arguments)?; @@ -156,7 +165,7 @@ impl WorkerService { .map_err(store_fault("tools/call:node.create"))?, }) .map_err(store_fault("tools/call:node.create"))?; - tool_success(&node) + tool_success(&node, render) } "change.record" => { let args = deserialize::<ChangeRecordToolArgs>(arguments)?; @@ -197,7 +206,7 @@ impl WorkerService { .map_err(store_fault("tools/call:change.record"))?, }) .map_err(store_fault("tools/call:change.record"))?; - tool_success(&node) + tool_success(&node, render) } "node.list" => { let args = deserialize::<NodeListToolArgs>(arguments)?; @@ -220,7 +229,7 @@ impl WorkerService { limit: args.limit.unwrap_or(20), }) .map_err(store_fault("tools/call:node.list"))?; - tool_success(&nodes) + tool_success(&nodes, render) } "node.read" => { let args = deserialize::<NodeReadToolArgs>(arguments)?; @@ -238,7 +247,7 @@ impl WorkerService { format!("node {node_id} was not found"), ) })?; - tool_success(&node) + tool_success(&node, render) } "node.annotate" => { let args = deserialize::<NodeAnnotateToolArgs>(arguments)?; @@ -265,7 +274,7 @@ impl WorkerService { annotation, ) .map_err(store_fault("tools/call:node.annotate"))?; - tool_success(&json!({"annotated": args.node_id})) + tool_success(&json!({"annotated": args.node_id}), render) } "node.archive" => { let args = deserialize::<NodeArchiveToolArgs>(arguments)?; @@ -275,7 +284,7 @@ impl WorkerService { .map_err(store_fault("tools/call:node.archive"))?, ) .map_err(store_fault("tools/call:node.archive"))?; - tool_success(&json!({"archived": args.node_id})) + tool_success(&json!({"archived": args.node_id}), render) } "note.quick" => { let args = deserialize::<QuickNoteToolArgs>(arguments)?; @@ -303,7 +312,7 @@ impl WorkerService { .map_err(store_fault("tools/call:note.quick"))?, }) .map_err(store_fault("tools/call:note.quick"))?; - tool_success(&node) + tool_success(&node, render) } "research.record" => { let args = deserialize::<ResearchRecordToolArgs>(arguments)?; @@ -335,7 +344,7 @@ impl WorkerService { .map_err(store_fault("tools/call:research.record"))?, }) .map_err(store_fault("tools/call:research.record"))?; - tool_success(&node) + tool_success(&node, render) } "experiment.close" => { let args = deserialize::<ExperimentCloseToolArgs>(arguments)?; @@ -420,7 +429,7 @@ impl WorkerService { .map_err(store_fault("tools/call:experiment.close"))?, }) .map_err(store_fault("tools/call:experiment.close"))?; - tool_success(&receipt) + tool_success(&receipt, render) } other => Err(FaultRecord::new( FaultKind::InvalidInput, @@ -501,16 +510,8 @@ fn deserialize<T: for<'de> Deserialize<'de>>(value: Value) -> Result<T, FaultRec }) } -fn tool_success(value: &impl serde::Serialize) -> Result<Value, FaultRecord> { - Ok(json!({ - "content": [{ - "type": "text", - "text": crate::to_pretty_json(value).map_err(store_fault("worker.tool_success"))?, - }], - "structuredContent": serde_json::to_value(value) - .map_err(store_fault("worker.tool_success"))?, - "isError": false, - })) +fn tool_success(value: &impl serde::Serialize, render: RenderMode) -> Result<Value, FaultRecord> { + crate::mcp::output::tool_success(value, render, FaultStage::Worker, "worker.tool_success") } fn store_fault<E>(operation: &'static str) -> impl FnOnce(E) -> FaultRecord diff --git a/crates/fidget-spinner-cli/tests/mcp_hardening.rs b/crates/fidget-spinner-cli/tests/mcp_hardening.rs index 8d3cd9d..1c70562 100644 --- a/crates/fidget-spinner-cli/tests/mcp_hardening.rs +++ b/crates/fidget-spinner-cli/tests/mcp_hardening.rs @@ -8,6 +8,7 @@ use clap as _; use dirs as _; use fidget_spinner_core::NonEmptyText; use fidget_spinner_store_sqlite::{ListNodesQuery, ProjectStore}; +use libmcp as _; use serde as _; use serde_json::{Value, json}; use time as _; @@ -160,6 +161,13 @@ fn tool_content(response: &Value) -> &Value { &response["result"]["structuredContent"] } +fn tool_text(response: &Value) -> Option<&str> { + response["result"]["content"] + .as_array() + .and_then(|content| content.first()) + .and_then(|entry| entry["text"].as_str()) +} + #[test] fn cold_start_exposes_health_and_telemetry() -> TestResult { let project_root = temp_project_root("cold_start")?; @@ -214,6 +222,29 @@ fn cold_start_exposes_health_and_telemetry() -> TestResult { } #[test] +fn tool_output_defaults_to_porcelain_and_supports_json_render() -> TestResult { + let project_root = temp_project_root("render_modes")?; + init_project(&project_root)?; + + let mut harness = McpHarness::spawn(None, &[])?; + let _ = harness.initialize()?; + harness.notify_initialized()?; + let bind = harness.bind_project(21, &project_root)?; + assert_eq!(bind["result"]["isError"].as_bool(), Some(false)); + + let porcelain = harness.call_tool(22, "project.status", json!({}))?; + let porcelain_text = must_some(tool_text(&porcelain), "porcelain project.status text")?; + assert!(porcelain_text.contains("project_root:")); + assert!(!porcelain_text.contains("\"project_root\":")); + + let json_render = harness.call_tool(23, "project.status", json!({"render": "json"}))?; + let json_text = must_some(tool_text(&json_render), "json project.status text")?; + assert!(json_text.contains("\"project_root\":")); + assert!(json_text.trim_start().starts_with('{')); + Ok(()) +} + +#[test] fn safe_request_retries_after_worker_crash() -> TestResult { let project_root = temp_project_root("crash_retry")?; init_project(&project_root)?; |