From 5df733a22b3a555646297f98b194143844e08b2c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 18 Sep 2026 10:57:07 -0400 Subject: [PATCH] Move fabro-mcp's pebble mapping and test client into fabro-cli `fabro-mcp` held two things after the legacy executor went: the mapping from Fabro's MCP server settings to the servers pebble starts, which only `fabro exec` still uses, and a stdio MCP client the tests of Fabro's own MCP server speak through. The mapping is now `fabro-cli`'s `mcp_servers` module and the client its test support's `McpStdioTestClient`; the crate is deleted. Its `config` module was a re-export of `fabro_types::settings::run`, which callers import directly. Co-Authored-By: Claude Fable 5.1 --- AGENTS.md | 4 +- Cargo.lock | 15 +-- docs/internal/logging-strategy.md | 7 -- lib/apps/fabro-cli/Cargo.toml | 3 +- lib/apps/fabro-cli/src/commands/exec.rs | 5 +- lib/apps/fabro-cli/src/main.rs | 1 + .../fabro-cli/src/mcp_servers.rs} | 9 +- lib/apps/fabro-cli/tests/it/cmd/mcp.rs | 6 +- .../fabro-cli/tests/it/support/mcp_client.rs} | 19 ++- lib/apps/fabro-cli/tests/it/support/mod.rs | 2 + lib/components/fabro-mcp/Cargo.toml | 35 ------ lib/components/fabro-mcp/src/config.rs | 1 - lib/components/fabro-mcp/src/lib.rs | 12 -- .../fabro-mcp/tests/test_mcp_server.py | 111 ------------------ 14 files changed, 25 insertions(+), 205 deletions(-) rename lib/{components/fabro-mcp/src/pebble.rs => apps/fabro-cli/src/mcp_servers.rs} (96%) rename lib/{components/fabro-mcp/src/test_support.rs => apps/fabro-cli/tests/it/support/mcp_client.rs} (92%) delete mode 100644 lib/components/fabro-mcp/Cargo.toml delete mode 100644 lib/components/fabro-mcp/src/config.rs delete mode 100644 lib/components/fabro-mcp/src/lib.rs delete mode 100644 lib/components/fabro-mcp/tests/test_mcp_server.py diff --git a/AGENTS.md b/AGENTS.md index ef19a66f8..2434df686 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -130,7 +130,7 @@ Fabro is an AI-powered workflow orchestration platform. Workflows are defined as - **fabro-llm** — Unified LLM client with providers: Anthropic, OpenAI, Gemini, OpenAI-compatible, plus retry/middleware/streaming - **fabro-api** — Auto-generated Rust types and reqwest HTTP client from OpenAPI spec (build.rs + progenitor) - **fabro-github** — GitHub App auth (JWT signing, installation tokens, PR creation) -- **fabro-mcp** — Model Context Protocol client/server +- **fabro-mcp-server** — Fabro's own MCP server (`fabro mcp`): the run tools for external agents - **fabro-slack** — Slack integration (socket mode, blocks API) - **fabro-checkpoint** — Git checkpoint author identity and commit trailers - **fabro-telemetry** — CLI analytics (Segment) and crash reporting (Sentry), with anonymous IDs, command sanitization, and detached subprocess delivery @@ -249,7 +249,7 @@ Fabro is an AI-powered workflow orchestration platform. Workflows are defined as - **fabro-llm** — Unified LLM client with providers: Anthropic, OpenAI, Gemini, OpenAI-compatible, plus retry/middleware/streaming - **fabro-api** — Auto-generated Rust types and reqwest HTTP client from OpenAPI spec (build.rs + progenitor) - **fabro-github** — GitHub App auth (JWT signing, installation tokens, PR creation) -- **fabro-mcp** — Model Context Protocol client/server +- **fabro-mcp-server** — Fabro's own MCP server (`fabro mcp`): the run tools for external agents - **fabro-slack** — Slack integration (socket mode, blocks API) - **fabro-checkpoint** — Git checkpoint author identity and commit trailers - **fabro-telemetry** — CLI analytics (Segment) and crash reporting (Sentry), with anonymous IDs, command sanitization, and detached subprocess delivery diff --git a/Cargo.lock b/Cargo.lock index 179c4fcd1..f7a068528 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2407,7 +2407,6 @@ dependencies = [ "fabro-llm", "fabro-macros", "fabro-manifest", - "fabro-mcp", "fabro-mcp-server", "fabro-oauth", "fabro-petri", @@ -2450,6 +2449,7 @@ dependencies = [ "regex", "reqwest 0.13.4", "ring", + "rmcp", "rustls", "sandbox-driver", "scopeguard", @@ -2754,19 +2754,6 @@ dependencies = [ "tracing-subscriber", ] -[[package]] -name = "fabro-mcp" -version = "0.357.0-nightly.0" -dependencies = [ - "anyhow", - "fabro-types", - "pebble-coding-agent", - "rmcp", - "serde_json", - "tokio", - "tracing", -] - [[package]] name = "fabro-mcp-server" version = "0.357.0-nightly.0" diff --git a/docs/internal/logging-strategy.md b/docs/internal/logging-strategy.md index f44ad46f7..ce47bda63 100644 --- a/docs/internal/logging-strategy.md +++ b/docs/internal/logging-strategy.md @@ -161,13 +161,6 @@ debug!(node = %id, handler = %handler_type, "Executing pipeline node"); debug!(node = %id, duration_ms = elapsed, "Pipeline node complete"); ``` -**fabro-mcp:** -```rust -info!(server = %name, tools = tool_count, "MCP server ready"); -debug!(server = %name, transport = %transport_type, "Connecting to MCP server"); -error!(server = %name, error = %err, "MCP server failed to start"); -``` - ## Cross-Package Guidelines Every crate that does meaningful work should emit tracing events. The `tracing` dependency is workspace-level — add it to any crate's `Cargo.toml` with: diff --git a/lib/apps/fabro-cli/Cargo.toml b/lib/apps/fabro-cli/Cargo.toml index b2e0aa74a..b7ec71fd2 100644 --- a/lib/apps/fabro-cli/Cargo.toml +++ b/lib/apps/fabro-cli/Cargo.toml @@ -31,7 +31,6 @@ sandbox-driver.workspace = true fabro-dump = { path = "../../components/fabro-dump" } fabro-install = { path = "../../components/fabro-install" } fabro-interview = { path = "../../components/fabro-interview" } -fabro-mcp = { path = "../../components/fabro-mcp" } fabro-mcp-server = { path = "../fabro-mcp-server" } fabro-petri = { path = "../../components/fabro-petri" } fabro-manifest = { path = "../../components/fabro-manifest" } @@ -120,7 +119,7 @@ assert_cmd = "2" fabro-db = { path = "../../foundation/fabro-db" } walkdir.workspace = true fabro-acp = { path = "../../components/fabro-acp", features = ["test-support"] } -fabro-mcp = { path = "../../components/fabro-mcp", features = ["test-support"] } +rmcp = { workspace = true, features = ["client", "transport-child-process"] } fabro-build-support = { path = "../../foundation/build-support" } fabro-sandbox = { path = "../../components/fabro-sandbox", features = ["test-support"] } fabro-server = { path = "../fabro-server", features = ["test-support"] } diff --git a/lib/apps/fabro-cli/src/commands/exec.rs b/lib/apps/fabro-cli/src/commands/exec.rs index 8e8101319..190b48b4a 100644 --- a/lib/apps/fabro-cli/src/commands/exec.rs +++ b/lib/apps/fabro-cli/src/commands/exec.rs @@ -22,12 +22,10 @@ use fabro_llm::gateway::{GatewayAdapter, GatewayError, GatewayTransport}; use fabro_llm::lithos_catalog::{Catalog, CatalogProvider}; use fabro_llm::middleware::{Call, Middleware, Next, Output}; use fabro_llm::{Client, ClientOptions, Error as LlmError, ErrorKind}; -use fabro_mcp::config::McpServerSettings; -use fabro_mcp::pebble::pebble_servers; use fabro_sandbox::{RunSandbox, SecretRedactor, local_sandbox}; use fabro_static::EnvVars; use fabro_types::settings::cli::OutputFormat as SettingsOutputFormat; -use fabro_types::settings::run::ResolvedMcpEntry; +use fabro_types::settings::run::{McpServerSettings, ResolvedMcpEntry}; use fabro_util::exit::{self, ErrorExt, ExitClass}; use fabro_util::home::Home; use fabro_util::terminal::Styles; @@ -45,6 +43,7 @@ use tokio_util::sync::CancellationToken; use crate::args::{AgentArgs, ExecArgs, ExecOutputFormat}; use crate::command_context::CommandContext; +use crate::mcp_servers::pebble_servers; #[cfg(feature = "sleep_inhibitor")] use crate::sleep_inhibitor; use crate::{server_client, user_config}; diff --git a/lib/apps/fabro-cli/src/main.rs b/lib/apps/fabro-cli/src/main.rs index ea7fb44db..befac102b 100644 --- a/lib/apps/fabro-cli/src/main.rs +++ b/lib/apps/fabro-cli/src/main.rs @@ -11,6 +11,7 @@ mod landing; mod local_server; mod logging; mod manifest_args; +mod mcp_servers; mod server_client; mod server_runs; mod shared; diff --git a/lib/components/fabro-mcp/src/pebble.rs b/lib/apps/fabro-cli/src/mcp_servers.rs similarity index 96% rename from lib/components/fabro-mcp/src/pebble.rs rename to lib/apps/fabro-cli/src/mcp_servers.rs index ab4f4ac55..797ee3e5a 100644 --- a/lib/components/fabro-mcp/src/pebble.rs +++ b/lib/apps/fabro-cli/src/mcp_servers.rs @@ -1,4 +1,4 @@ -//! Fabro's MCP server settings as the servers pebble starts. +//! Fabro's MCP server settings as the servers pebble starts, for `fabro exec`. //! //! Fabro's three transports are pebble's three placements: a `stdio` server //! is a child of fabro's process, an `http` server is reached directly, and a @@ -8,16 +8,15 @@ use std::collections::{BTreeMap, HashMap}; +use fabro_types::settings::run::{McpHttpProtocol, McpServerSettings, McpTransport}; use pebble_coding_agent::mcp::{McpHttpProtocol as PebbleProtocol, McpPlacement, McpServer}; -use crate::config::{McpHttpProtocol, McpServerSettings, McpTransport}; - /// Where a sandbox-hosted SSE server serves its event stream. const SSE_PATH: &str = "/sse"; /// The pebble server `settings` describes. #[must_use] -pub fn pebble_server(settings: &McpServerSettings) -> McpServer { +pub(crate) fn pebble_server(settings: &McpServerSettings) -> McpServer { let placement = match &settings.transport { McpTransport::Stdio { command, env } => McpPlacement::Stdio { command: command.clone(), @@ -53,7 +52,7 @@ pub fn pebble_server(settings: &McpServerSettings) -> McpServer { } /// The pebble servers for every configured server, in configuration order. -pub fn pebble_servers<'a>( +pub(crate) fn pebble_servers<'a>( settings: impl IntoIterator, ) -> Vec { settings.into_iter().map(pebble_server).collect() diff --git a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs index 246e7b99b..98d57079e 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs @@ -18,16 +18,16 @@ use std::time::{Duration, Instant}; use chrono::{DateTime, Duration as ChronoDuration, Utc}; use fabro_client::{AuthEntry, AuthStore, DevTokenEntry, OAuthEntry, StoredSubject}; -use fabro_mcp::config::{McpServerSettings, McpTransport}; -use fabro_mcp::test_support::McpStdioTestClient as McpClient; use fabro_test::{fabro_json_snapshot, fabro_snapshot, test_context}; +use fabro_types::settings::run::{McpServerSettings, McpTransport}; use fabro_types::{Graph, RunId, WorkflowSettings, test_support}; use httpmock::Method::{GET, POST}; use httpmock::MockServer; use super::support::{mock_resolved_run, remote_run_summary_json}; use crate::support::{ - RealAuthHarness, TEST_DEV_TOKEN, run_projection_json, seed_dev_token_auth, unique_run_id, + McpStdioTestClient as McpClient, RealAuthHarness, TEST_DEV_TOKEN, run_projection_json, + seed_dev_token_auth, unique_run_id, }; const MCP_RUN_TOOL_NAMES: &[&str] = &[ diff --git a/lib/components/fabro-mcp/src/test_support.rs b/lib/apps/fabro-cli/tests/it/support/mcp_client.rs similarity index 92% rename from lib/components/fabro-mcp/src/test_support.rs rename to lib/apps/fabro-cli/tests/it/support/mcp_client.rs index 0d29e4537..8703e46fd 100644 --- a/lib/components/fabro-mcp/src/test_support.rs +++ b/lib/apps/fabro-cli/tests/it/support/mcp_client.rs @@ -1,15 +1,16 @@ -//! A stdio MCP client for tests of fabro's own MCP server. +//! A stdio MCP client for the tests of fabro's own MCP server. //! //! Production agents reach MCP servers through pebble, which owns the client. //! Fabro's `fabro mcp` command *is* an MCP server, and its tests need a //! client to speak to it over its standard streams; this is that client and -//! nothing more. It links only into tests. +//! nothing more. use std::process::Stdio; use std::sync::Arc; use std::time::Duration; use anyhow::{Context as _, Result, anyhow}; +use fabro_types::settings::run::{McpServerSettings, McpTransport}; use rmcp::model::{ CallToolRequestParams, CallToolResult, ClientCapabilities, ClientInfo, Implementation, ProtocolVersion, @@ -20,8 +21,6 @@ use tokio::process::Command; use tokio::sync::Mutex; use tokio::time; -use crate::config::{McpServerSettings, McpTransport}; - enum State { Connecting(Option), Ready(Arc>), @@ -29,7 +28,7 @@ enum State { } /// A test client over a stdio MCP server. -pub struct McpStdioTestClient { +pub(crate) struct McpStdioTestClient { server_name: String, state: Mutex, } @@ -37,7 +36,7 @@ pub struct McpStdioTestClient { impl McpStdioTestClient { /// Spawns the server `config` names. Only a `stdio` transport is /// supported; call [`initialize`](Self::initialize) next. - pub fn new(config: &McpServerSettings) -> Result { + pub(crate) fn new(config: &McpServerSettings) -> Result { let McpTransport::Stdio { command, env } = &config.transport else { return Err(anyhow!( "MCP test client '{}': only a stdio transport is supported", @@ -73,7 +72,7 @@ impl McpStdioTestClient { } /// Performs the MCP handshake within `timeout`. - pub async fn initialize(&self, timeout: Duration) -> Result<()> { + pub(crate) async fn initialize(&self, timeout: Duration) -> Result<()> { let transport = { let mut guard = self.state.lock().await; match &mut *guard { @@ -116,7 +115,7 @@ impl McpStdioTestClient { } /// Every tool the server exposes, as `(name, description, input_schema)`. - pub async fn list_tools(&self) -> Result> { + pub(crate) async fn list_tools(&self) -> Result> { let service = self.service().await?; let tools = service.list_all_tools().await.map_err(|error| { anyhow!( @@ -137,7 +136,7 @@ impl McpStdioTestClient { } /// Calls `name` with `arguments`, waiting at most `timeout`. - pub async fn call_tool( + pub(crate) async fn call_tool( &self, name: &str, arguments: serde_json::Value, @@ -171,7 +170,7 @@ impl McpStdioTestClient { } /// Ends the session and stops the server. - pub async fn shutdown(self) -> Result<()> { + pub(crate) async fn shutdown(self) -> Result<()> { let service = match std::mem::replace(&mut *self.state.lock().await, State::Closed) { State::Connecting(_) | State::Closed => None, State::Ready(service) => Some(service), diff --git a/lib/apps/fabro-cli/tests/it/support/mod.rs b/lib/apps/fabro-cli/tests/it/support/mod.rs index a8b0c7be7..663dc17c1 100644 --- a/lib/apps/fabro-cli/tests/it/support/mod.rs +++ b/lib/apps/fabro-cli/tests/it/support/mod.rs @@ -1,6 +1,7 @@ use fabro_types::{PetriAdmission, test_support}; mod auth_harness; mod auth_tokens; +mod mcp_client; use assert_cmd::Command; pub(crate) use auth_harness::{ @@ -11,6 +12,7 @@ pub(crate) use auth_tokens::{TEST_SESSION_SECRET, issue_test_github_jwt, issue_t use fabro_store::EventEnvelope; use fabro_test::{EnvVars, TestContext, preserve_coverage_env}; use fabro_types::{Graph, RunId, RunSpec, WorkflowSettings}; +pub(crate) use mcp_client::McpStdioTestClient; pub(crate) fn run_output_filters(context: &TestContext) -> Vec<(String, String)> { let mut filters = context.filters(); diff --git a/lib/components/fabro-mcp/Cargo.toml b/lib/components/fabro-mcp/Cargo.toml deleted file mode 100644 index 5d5a74dd1..000000000 --- a/lib/components/fabro-mcp/Cargo.toml +++ /dev/null @@ -1,35 +0,0 @@ -[package] -name = "fabro-mcp" -edition.workspace = true -version.workspace = true -publish = false -license.workspace = true -description = "MCP server settings for fabro's agents, and how they reach pebble" - -[lib] -doctest = false - -[features] -# A stdio MCP client for tests of fabro's own MCP server. Production agents -# reach MCP servers through pebble; nothing here links into a normal build. -test-support = ["dep:anyhow", "dep:rmcp", "dep:serde_json", "dep:tokio", "dep:tracing"] - -[lints] -workspace = true - -[dependencies] -fabro-types = { path = "../../foundation/fabro-types" } -pebble-coding-agent.workspace = true -anyhow = { workspace = true, optional = true } -rmcp = { workspace = true, features = ["client", "transport-child-process"], optional = true } -serde_json = { workspace = true, optional = true } -tokio = { workspace = true, optional = true } -tracing = { workspace = true, optional = true } - -[dev-dependencies] -# The test-support client's dependencies, so `cfg(test)` builds see them too. -anyhow.workspace = true -rmcp = { workspace = true, features = ["client", "transport-child-process"] } -serde_json.workspace = true -tokio.workspace = true -tracing.workspace = true diff --git a/lib/components/fabro-mcp/src/config.rs b/lib/components/fabro-mcp/src/config.rs deleted file mode 100644 index b7a6dccf9..000000000 --- a/lib/components/fabro-mcp/src/config.rs +++ /dev/null @@ -1 +0,0 @@ -pub use fabro_types::settings::run::{McpHttpProtocol, McpServerSettings, McpTransport}; diff --git a/lib/components/fabro-mcp/src/lib.rs b/lib/components/fabro-mcp/src/lib.rs deleted file mode 100644 index 2cf3828f4..000000000 --- a/lib/components/fabro-mcp/src/lib.rs +++ /dev/null @@ -1,12 +0,0 @@ -//! MCP server settings for fabro's agents, and how they reach pebble. -//! -//! Fabro configures MCP servers in its settings ([`config`]); pebble starts -//! them, registers their tools, and closes them with the agent. [`pebble`] -//! maps one to the other. The stdio client fabro once ran itself is gone; -//! [`test_support`] keeps a small one for the tests of fabro's own MCP server. - -pub mod config; -pub mod pebble; - -#[cfg(any(test, feature = "test-support"))] -pub mod test_support; diff --git a/lib/components/fabro-mcp/tests/test_mcp_server.py b/lib/components/fabro-mcp/tests/test_mcp_server.py deleted file mode 100644 index c3dd8838d..000000000 --- a/lib/components/fabro-mcp/tests/test_mcp_server.py +++ /dev/null @@ -1,111 +0,0 @@ -#!/usr/bin/env python3 -"""Minimal MCP server for integration testing over stdio. - -Speaks JSON-RPC 2.0 over stdin/stdout per the MCP specification. -Exposes a single tool: echo(message) -> message. -""" -import json -import os -import sys -import time - -SERVER_INFO = { - "name": "test-echo-server", - "version": "0.1.0", -} - -TOOL = { - "name": "echo", - "description": "Echo back the message", - "inputSchema": { - "type": "object", - "properties": { - "message": {"type": "string", "description": "Message to echo"} - }, - "required": ["message"], - }, -} - - -def handle_request(req): - method = req.get("method") - req_id = req.get("id") - params = req.get("params", {}) - - if method == "initialize": - return { - "jsonrpc": "2.0", - "id": req_id, - "result": { - "protocolVersion": "2025-03-26", - "capabilities": {"tools": {}}, - "serverInfo": SERVER_INFO, - }, - } - - if method == "tools/list": - return { - "jsonrpc": "2.0", - "id": req_id, - "result": {"tools": [TOOL]}, - } - - if method == "tools/call": - tool_name = params.get("name") - arguments = params.get("arguments", {}) - if tool_name == "echo": - msg = arguments.get("message", "") - if msg == "__cwd__": - msg = os.getcwd() - elif msg.startswith("__env:") and msg.endswith("__"): - key = msg[len("__env:") : -len("__")] - msg = os.environ.get(key, "") - elif msg.startswith("__sleep_ms:") and msg.endswith("__"): - milliseconds = int(msg[len("__sleep_ms:") : -len("__")]) - time.sleep(milliseconds / 1000) - msg = f"slept {milliseconds}ms" - return { - "jsonrpc": "2.0", - "id": req_id, - "result": { - "content": [{"type": "text", "text": msg}], - }, - } - return { - "jsonrpc": "2.0", - "id": req_id, - "result": { - "content": [{"type": "text", "text": f"unknown tool: {tool_name}"}], - "isError": True, - }, - } - - # Notifications (no id) — just ignore - if req_id is None: - return None - - return { - "jsonrpc": "2.0", - "id": req_id, - "error": {"code": -32601, "message": f"Method not found: {method}"}, - } - - -def main(): - for line in sys.stdin: - line = line.strip() - if not line: - continue - try: - req = json.loads(line) - except json.JSONDecodeError: - continue - - resp = handle_request(req) - if resp is not None: - sys.stdout.write(json.dumps(resp) + "\n") - sys.stdout.flush() - - -if __name__ == "__main__": - main()