Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions frontend/src/api/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
28 changes: 28 additions & 0 deletions frontend/src/views/connections/McpServersSection.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,34 @@ export function McpServersSection({ client, company, canManage, chrome = "inline
</span>
</div>
<p className="truncate text-xs text-muted-foreground">{server.endpoint}</p>
{/* 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 ? (
<p
data-testid="mcp-reachability-none"
className="flex items-start gap-1.5 rounded-md border border-destructive/30 bg-destructive/10 px-2 py-1 text-xs font-medium text-destructive"
>
<AlertTriangle className="mt-0.5 size-3.5 shrink-0" />
<span>
No agent can reach this server — no teammate&apos;s tool grants cover{" "}
<code className="font-mono">mcp:{server.name}</code>. Widen a company or
per-agent tool grant, or this server is unused.
</span>
</p>
) : (
<p data-testid="mcp-reachability" className="text-xs text-muted-foreground">
Reachable by:{" "}
<span className="font-medium text-foreground">
{server.reachableBy.join(", ")}
</span>
</p>
))}
{health && health.status !== "ok" && health.message && (
<p className="text-xs text-muted-foreground">{health.message}</p>
)}
Expand Down
10 changes: 1 addition & 9 deletions src/harness/mcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -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).
Expand Down
13 changes: 13 additions & 0 deletions src/runtime/tools.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Vec<ToolSpec>> {
Expand Down
114 changes: 107 additions & 7 deletions src/server/ops/mcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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<String>,
/// The last recorded probe outcome (scrubbed), or `None` when never probed.
#[serde(skip_serializing_if = "Option::is_none")]
health: Option<McpHealth>,
Expand Down Expand Up @@ -216,9 +232,13 @@ async fn manifest_servers(runtime: &CompanyRuntime) -> Result<Vec<McpServer>, 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<McpHealth>) -> 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<String>,
health: Option<McpHealth>,
) -> McpServerDto {
McpServerDto {
name: decl.name.clone(),
endpoint: decl.endpoint.clone(),
Expand All @@ -229,24 +249,93 @@ fn dto_from_decl(decl: &mcp::McpServerDecl, health: Option<McpHealth>) -> 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<String>)> {
let allow = &record.manifest.tools.allow;
let mut grants: Vec<(String, Vec<String>)> = 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:<slug>` 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<String>)], decl: &mcp::McpServerDecl) -> Vec<String> {
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<Json<Vec<McpServerDto>>, 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))
}
Expand Down Expand Up @@ -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)?;
Expand All @@ -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,
Expand Down
Loading
Loading