diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 8b012f84a..dea281334 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -751,6 +751,17 @@ export interface McpServer { timeoutSecs: number; /** Whether an outbound credential is stored — never the credential itself. */ authConfigured: boolean; + /** + * Ids of the company's agents whose effective tool grants cover this server — + * who can actually call it (issue #568). On an **enabled** server an empty + * array means no teammate can reach it, a probable misconfiguration the + * console flags loudly. A **disabled** server is always empty (the harness + * hands out no tool for it whatever the grants say), so the console reads the + * empty case against `enabled` and stays quiet there. Optional only for + * forward-compat with an older backend that does not send the field; + * `undefined` (unknown) is treated differently from `[]` (known-empty). + */ + reachableBy?: string[]; /** The last recorded (scrubbed) probe outcome, when the server has been probed. */ health?: McpHealth; } diff --git a/frontend/src/views/connections/McpServersSection.tsx b/frontend/src/views/connections/McpServersSection.tsx index 3493da6cc..a310de1d8 100644 --- a/frontend/src/views/connections/McpServersSection.tsx +++ b/frontend/src/views/connections/McpServersSection.tsx @@ -444,6 +444,34 @@ export function McpServersSection({ client, company, canManage, chrome = "inline

{server.endpoint}

+ {/* Reachability (issue #568): who can actually call this server. An + enabled server no agent reaches is almost always a misconfiguration, + so that empty case is flagged loudly rather than shown as a blank list. + A disabled server is empty by construction — the harness hands out no + tool for it whatever the grants say — so the loud state is scoped to + enabled servers; flagging an off server would cry wolf on intent. */} + {server.reachableBy !== undefined && + server.enabled && + (server.reachableBy.length === 0 ? ( +

+ + + No agent can reach this server — no teammate's tool grants cover{" "} + mcp:{server.name}. Widen a company or + per-agent tool grant, or this server is unused. + +

+ ) : ( +

+ Reachable by:{" "} + + {server.reachableBy.join(", ")} + +

+ ))} {health && health.status !== "ok" && health.message && (

{health.message}

)} diff --git a/src/harness/mcp.rs b/src/harness/mcp.rs index 3721cb8a7..f1c5ce352 100644 --- a/src/harness/mcp.rs +++ b/src/harness/mcp.rs @@ -36,7 +36,7 @@ use crate::error::OpenCompanyError; use crate::harness::mcp_probe::{ McpFailure, McpFailureQueue, classify_mcp_error, operator_message, scrub, strip_endpoint, }; -use crate::runtime::tools::grant_matches; +use crate::runtime::tools::{grant_matches, grants_cover_server}; /// Builds a registry from a set of decls, keeping only the enabled ones. /// @@ -88,14 +88,6 @@ pub fn registry_for_agent( } } -/// Whether an agent's effective `grants` cover the MCP server named `name`, -/// using the same glob semantics as every other tool grant (`mcp:*` = all, -/// `mcp:notion` = exact). -fn grants_cover_server(grants: &[String], name: &str) -> bool { - let want = format!("mcp:{name}"); - grants.iter().any(|grant| grant_matches(grant, &want)) -} - /// Whether `agent`'s tool grants reach the MCP server named `name`, using the /// same glob semantics as every other tool grant (`mcp:*` = all, `mcp:notion` = /// exact). diff --git a/src/runtime/tools.rs b/src/runtime/tools.rs index 734cb0d20..72185bca2 100644 --- a/src/runtime/tools.rs +++ b/src/runtime/tools.rs @@ -89,6 +89,19 @@ pub(crate) fn grant_matches(grant: &str, tool: &str) -> bool { grant == tool } +/// Whether an agent's effective tool `grants` cover the MCP server named `name`, +/// using the same glob semantics as every other grant (`mcp:*` grants all, +/// `mcp:notion` is exact). The single primitive read by both the harness's +/// per-agent registry assembly (`registry_for_agent`) and the console's +/// reachability view (issue #568), so the two can never disagree about which +/// agents reach a server. `grants` are the *effective* grants — resolve them +/// with [`agent_effective_grants`](crate::runtime::builder::agent_effective_grants) +/// first, never the raw per-agent `tools`. +pub(crate) fn grants_cover_server(grants: &[String], name: &str) -> bool { + let want = format!("mcp:{name}"); + grants.iter().any(|grant| grant_matches(grant, &want)) +} + #[async_trait] impl ToolProvider for StubToolProvider { async fn catalog(&self, _company: &CompanyId) -> Result> { diff --git a/src/server/ops/mcp.rs b/src/server/ops/mcp.rs index 3b283ecde..afe9e3ad0 100644 --- a/src/server/ops/mcp.rs +++ b/src/server/ops/mcp.rs @@ -29,6 +29,9 @@ use crate::company::mcp::{ }; use crate::company::runtime::CompanyRuntime; use crate::error::OpenCompanyError; +use crate::ports::types::CompanyRecord; +use crate::runtime::builder::agent_effective_grants; +use crate::runtime::tools::grants_cover_server; use crate::server::error::ApiError; use crate::server::ops::{AdminScopedCompany, ScopedCompany, scoped}; @@ -67,6 +70,19 @@ struct McpServerDto { timeout_secs: u64, /// Whether an outbound credential is stored — never the credential itself. auth_configured: bool, + /// The ids of the company's agents whose effective tool grants cover this + /// server — who can actually call it (issue #568). Computed over the same + /// roster the harness builds (manifest agents + promoted overlay teammates), + /// through the shared + /// [`grants_cover_server`](crate::runtime::tools::grants_cover_server), so the + /// console cannot disagree with the harness about reachability. **An empty + /// list is meaningful**: an *enabled*, healthy server no teammate can reach + /// is almost always a misconfiguration, and the console flags it rather than + /// showing an empty list silently. A **disabled** server is always empty — + /// the harness hands out no tool for it whatever the grants say — so the + /// console reads the empty case against `enabled` and stays quiet there. + /// Always serialized (even when empty). + reachable_by: Vec, /// The last recorded probe outcome (scrubbed), or `None` when never probed. #[serde(skip_serializing_if = "Option::is_none")] health: Option, @@ -216,9 +232,13 @@ async fn manifest_servers(runtime: &CompanyRuntime) -> Result, Ap } /// Projects an effective decl (already merged + auth-resolved) to the console -/// DTO, reducing the resolved credential to a boolean and attaching the last -/// (scrubbed) probe health. -fn dto_from_decl(decl: &mcp::McpServerDecl, health: Option) -> McpServerDto { +/// DTO, reducing the resolved credential to a boolean, listing the agents that +/// can reach it (issue #568), and attaching the last (scrubbed) probe health. +fn dto_from_decl( + decl: &mcp::McpServerDecl, + reachable_by: Vec, + health: Option, +) -> McpServerDto { McpServerDto { name: decl.name.clone(), endpoint: decl.endpoint.clone(), @@ -229,24 +249,93 @@ fn dto_from_decl(decl: &mcp::McpServerDecl, health: Option) -> McpSer disallowed_tools: decl.disallowed_tools.clone(), timeout_secs: decl.timeout_secs, auth_configured: decl.auth.is_configured(), + reachable_by, health, } } +/// Every roster agent's *effective* tool grants (issue #568), as +/// `(agent_id, grants)`. The roster is exactly what the harness builds in +/// `build_roster`: the manifest agents (each with its own `tools` narrowed by +/// the company `allow`), plus the promoted overlay teammates. An overlay +/// teammate has no manifest `tools` row, so it inherits the full company `allow` +/// — the standard grant `overlay_agent_to_manifest` gives it — and an overlay id +/// already claimed by a manifest agent is skipped, both mirroring the harness so +/// console reachability equals what an agent is actually granted. +fn roster_grants(record: &CompanyRecord) -> Vec<(String, Vec)> { + let allow = &record.manifest.tools.allow; + let mut grants: Vec<(String, Vec)> = record + .manifest + .agents + .iter() + .map(|agent| { + ( + agent.id.clone(), + agent_effective_grants(allow, &agent.tools), + ) + }) + .collect(); + let manifest_ids: std::collections::HashSet<&str> = record + .manifest + .agents + .iter() + .map(|agent| agent.id.as_str()) + .collect(); + for overlay in &record.overlay_agents { + if manifest_ids.contains(overlay.id.as_str()) { + continue; + } + // No manifest tools row → the company's standard grant (empty `tools` + // ⇒ inherit `allow`), matching `overlay_agent_to_manifest`. + grants.push((overlay.id.clone(), agent_effective_grants(allow, &[]))); + } + grants +} + +/// The ids of the agents whose effective `grants` reach `decl` (issue #568), +/// read through the shared [`grants_cover_server`] so this agrees with the +/// harness registry. Empty ⇒ no teammate can reach the server. +/// +/// A **disabled** server reaches nobody regardless of grants: `registry_for_agent` +/// filters on `decl.enabled && grants_cover_server(..)`, so an agent granted +/// `mcp:` still gets no such tool while the server is off. Mirroring both +/// halves of that filter here is what keeps the console from claiming a +/// reachability the harness does not hand out. +fn reachers_of(roster_grants: &[(String, Vec)], decl: &mcp::McpServerDecl) -> Vec { + if !decl.enabled { + return Vec::new(); + } + roster_grants + .iter() + .filter(|(_, grants)| grants_cover_server(grants, &decl.name)) + .map(|(id, _)| id.clone()) + .collect() +} + /// `GET …/mcp/servers` — the company's effective MCP servers, each with its last /// recorded (scrubbed) probe health. async fn list_servers(company: ScopedCompany) -> Result>, ApiError> { let runtime = company.runtime.as_ref(); - let manifest = manifest_servers(runtime).await?; + // One record load feeds both the manifest servers (merged into the effective + // set) and the roster used for reachability (issue #568), rather than loading + // it twice. + let record = runtime.store().load(runtime.id()).await.map_err(ApiError)?; + let manifest = record + .as_ref() + .map(|r| r.manifest.mcp_servers.clone()) + .unwrap_or_default(); let decls = resolve_effective(runtime.id(), &manifest, runtime.secrets().as_ref()) .await .map_err(ApiError)?; + // Resolve every agent's effective grants once, then ask per server who is + // covered — the wildcard-heavy work happens N(agents) times, not N×M. + let grants = record.as_ref().map(roster_grants).unwrap_or_default(); let mut out = Vec::with_capacity(decls.len()); for decl in &decls { let health = load_health(runtime.id(), &decl.name, runtime.secrets().as_ref()) .await .map_err(ApiError)?; - out.push(dto_from_decl(decl, health)); + out.push(dto_from_decl(decl, reachers_of(&grants, decl), health)); } Ok(Json(out)) } @@ -451,7 +540,13 @@ async fn mutation_response( // DTO so the response and a later `GET` agree. let test = probe_and_persist(runtime, name).await; - let manifest = manifest_servers(runtime).await?; + // One record load: the manifest servers merged into the effective set, and + // the roster the mutated server's reachability is computed against (#568). + let record = runtime.store().load(runtime.id()).await.map_err(ApiError)?; + let manifest = record + .as_ref() + .map(|r| r.manifest.mcp_servers.clone()) + .unwrap_or_default(); let decls = resolve_effective(runtime.id(), &manifest, runtime.secrets().as_ref()) .await .map_err(ApiError)?; @@ -460,11 +555,16 @@ async fn mutation_response( "`{name}` not found" ))) })?; + let reachable_by = record + .as_ref() + .map(roster_grants) + .map(|grants| reachers_of(&grants, decl)) + .unwrap_or_default(); let health = load_health(runtime.id(), name, runtime.secrets().as_ref()) .await .map_err(ApiError)?; Ok(Json(MutationResponse { - server: dto_from_decl(decl, health), + server: dto_from_decl(decl, reachable_by, health), note: NEXT_TURN_NOTE.to_string(), test, warning, diff --git a/src/server/ops/write_test.rs b/src/server/ops/write_test.rs index cf320574c..4257531bc 100644 --- a/src/server/ops/write_test.rs +++ b/src/server/ops/write_test.rs @@ -2388,6 +2388,45 @@ async fn state_with_manifest(home: &std::path::Path, manifest: CompanyManifest) state } +/// Like [`state_with_manifest`], but seeds operator-added overlay teammates too, +/// so a test can assert MCP reachability over the full runtime roster — manifest +/// agents plus overlay agents — the way `build_roster` composes it (issue #568). +async fn state_with_manifest_and_overlays( + home: &std::path::Path, + manifest: CompanyManifest, + overlay_agents: Vec, +) -> AppState { + use crate::ports::CompanyStore; + let store = FsCompanyStore::new(home.to_path_buf()); + let id = CompanyId::new("acme"); + store + .save(&CompanyRecord { + id: id.clone(), + manifest: manifest.clone(), + ledger: Vec::new(), + lifecycle: "running".to_string(), + overlay_agents, + overlay_desk_members: Vec::new(), + overlay_desk_order: Vec::new(), + overlay_desks: Vec::new(), + overlay_workflows: Vec::new(), + overlay_budgets: Vec::new(), + disabled_workflows: Vec::new(), + template_provenance: None, + }) + .await + .unwrap(); + let runtime = RuntimeBuilder::new(home.to_path_buf(), manifest) + .with_id(id.clone()) + .build() + .await + .unwrap(); + let state = AppState::new(AppConfig::default()); + state.registry().insert(id, std::sync::Arc::new(runtime)); + crate::server::test_support::seed_fixed_admin(&state, "acme").await; + state +} + #[tokio::test] async fn mcp_servers_crud_round_trips_and_token_is_write_only() { let home_dir = home(); @@ -2516,6 +2555,166 @@ async fn mcp_manifest_server_cannot_be_deleted_but_can_be_overridden() { assert_eq!(status, StatusCode::OK); assert_eq!(updated["server"]["source"], "manifest"); assert_eq!(updated["server"]["enabled"], false); + // The mutating response carries reachability too (issue #568), so the console + // reflects who can reach the server right after an edit, not only on reload. + assert!( + updated["server"]["reachableBy"].is_array(), + "a mutating response also carries reachableBy" + ); +} + +/// Issue #568: each listed server carries the ids of the agents whose *effective* +/// grants reach it — over the full runtime roster, manifest agents plus overlay +/// teammates. With a company `allow = ["*"]`, an agent that declares no `tools` +/// (and every overlay teammate, which has no tools row) inherits the wildcard and +/// reaches everything; an agent that narrows itself to `mcp:notion` reaches only +/// that server. +#[tokio::test] +async fn mcp_reachability_lists_reaching_agents_including_overlay() { + let manifest: CompanyManifest = toml::from_str( + "[company]\nname = \"Acme\"\n[tools]\nallow = [\"*\"]\n\ + [[agent]]\nid = \"ceo\"\nrole = \"Chief\"\ntools = [\"mcp:notion\"]\n\ + [[agent]]\nid = \"eng\"\nrole = \"Engineer\"\n[policy]\nmode = \"full\"\n\ + [[mcp_server]]\nname = \"notion\"\nendpoint = \"https://notion.example/mcp\"\n\ + [[mcp_server]]\nname = \"linear\"\nendpoint = \"https://linear.example/mcp\"\n", + ) + .unwrap(); + let home_dir = home(); + let home = home_dir.path().to_path_buf(); + let overlay = crate::ports::types::OverlayAgent { + id: "helper".to_string(), + name: "Helper".to_string(), + role: "Assistant".to_string(), + description: None, + }; + let state = state_with_manifest_and_overlays(&home, manifest, vec![overlay]).await; + + let (status, list) = send(&state, "GET", "/api/v1/company/mcp/servers", None).await; + assert_eq!(status, StatusCode::OK); + let reach = |name: &str| -> Vec { + let row = list + .as_array() + .unwrap() + .iter() + .find(|s| s["name"] == name) + .unwrap_or_else(|| panic!("server `{name}` is listed")); + let mut ids: Vec = row["reachableBy"] + .as_array() + .expect("reachableBy serializes as an array") + .iter() + .map(|v| v.as_str().unwrap().to_string()) + .collect(); + ids.sort(); + ids + }; + + // notion: the narrowed ceo, the wildcard-inheriting eng, and the overlay. + assert_eq!(reach("notion"), vec!["ceo", "eng", "helper"]); + // linear: only the wildcard holders — ceo scoped itself out of it. + assert_eq!( + reach("linear"), + vec!["eng", "helper"], + "ceo narrowed to mcp:notion, so it cannot reach linear" + ); +} + +/// Issue #568: a server no agent's grants cover comes back with an **empty** +/// `reachableBy` — the signal the console flags loudly rather than showing a +/// healthy server that is silently unreachable. Here a narrow company +/// `allow = ["mcp:docs"]` reaches `docs` but never `notion`. +#[tokio::test] +async fn mcp_reachability_flags_a_server_no_agent_can_reach() { + let manifest: CompanyManifest = toml::from_str( + "[company]\nname = \"Acme\"\n[tools]\nallow = [\"mcp:docs\"]\n\ + [[agent]]\nid = \"ceo\"\nrole = \"Chief\"\ntools = [\"mcp:docs\"]\n[policy]\nmode = \"full\"\n\ + [[mcp_server]]\nname = \"docs\"\nendpoint = \"https://docs.example/mcp\"\n\ + [[mcp_server]]\nname = \"notion\"\nendpoint = \"https://notion.example/mcp\"\n", + ) + .unwrap(); + let home_dir = home(); + let home = home_dir.path().to_path_buf(); + let state = state_with_manifest(&home, manifest).await; + + let (status, list) = send(&state, "GET", "/api/v1/company/mcp/servers", None).await; + assert_eq!(status, StatusCode::OK); + let row = |name: &str| { + list.as_array() + .unwrap() + .iter() + .find(|s| s["name"] == name) + .unwrap_or_else(|| panic!("server `{name}` is listed")) + .clone() + }; + assert_eq!( + row("docs")["reachableBy"] + .as_array() + .unwrap() + .iter() + .map(|v| v.as_str().unwrap()) + .collect::>(), + vec!["ceo"], + "the company allow covers mcp:docs for the one agent" + ); + assert!( + row("notion")["reachableBy"].as_array().unwrap().is_empty(), + "no agent's grants cover mcp:notion — the flagged zero case" + ); +} + +/// Issue #568: a **disabled** server reaches nobody, however wide the grants. +/// `registry_for_agent` filters on `decl.enabled && grants_cover_server(..)`, so +/// an agent holding `mcp:docs` is handed no such tool while the server is off — +/// reporting it as reachable would be the console/harness disagreement this +/// feature exists to remove. Asserted on both readers: the mutating response +/// that turns the server off, and the later list. +#[tokio::test] +async fn mcp_reachability_is_empty_for_a_disabled_server() { + let manifest: CompanyManifest = toml::from_str( + "[company]\nname = \"Acme\"\n[tools]\nallow = [\"*\"]\n\ + [[agent]]\nid = \"ceo\"\nrole = \"Chief\"\ntools = [\"mcp:docs\"]\n[policy]\nmode = \"full\"\n\ + [[mcp_server]]\nname = \"docs\"\nendpoint = \"https://docs.example/mcp\"\n", + ) + .unwrap(); + let home_dir = home(); + let home = home_dir.path().to_path_buf(); + let state = state_with_manifest(&home, manifest).await; + + let reach = |body: &serde_json::Value| -> Vec { + body["reachableBy"] + .as_array() + .expect("reachableBy serializes as an array") + .iter() + .map(|v| v.as_str().unwrap().to_string()) + .collect() + }; + + // Enabled: the one agent's grant covers it. + let (status, list) = send(&state, "GET", "/api/v1/company/mcp/servers", None).await; + assert_eq!(status, StatusCode::OK); + assert_eq!(reach(&list[0]), vec!["ceo".to_string()]); + + // Disabling it empties reachability in the mutating response itself. + let (status, updated) = send( + &state, + "PUT", + "/api/v1/company/mcp/servers/docs", + Some(json!({ "enabled": false })), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(updated["server"]["enabled"], false); + assert!( + reach(&updated["server"]).is_empty(), + "a disabled server is handed to no agent, so it is reachable by none" + ); + + // And the list agrees on the next read — the grant is unchanged, the server is off. + let (_, list) = send(&state, "GET", "/api/v1/company/mcp/servers", None).await; + assert_eq!(list[0]["enabled"], false); + assert!( + reach(&list[0]).is_empty(), + "the list reader applies the same enabled filter as the harness" + ); } /// Without the `openhuman` feature there is no MCP transport, so live discovery