feat(mcp): surface which agents can reach each MCP server (#568) - #631
Conversation
…ai#568) The MCP console showed a server's health and tool count but never who could actually call it. An agent reaches `mcp:<slug>` only when its effective grants cover it, so the moment a company narrows grants per agent a server can be live, healthy, and reachable by nobody — silently, with no error anywhere in the chain (such an agent doesn't even get `mcp_list_servers`). GET .../mcp/servers (and every mutating response) now carries `reachableBy`: the ids of the agents whose effective grants cover the server. The console renders it per row and flags the empty case loudly, since a healthy server no teammate can reach is almost always a misconfiguration rather than intent. Reachability is computed over the same roster the harness builds — manifest agents plus promoted overlay teammates — using the exact machinery the harness registry uses, so the console can't disagree with what an agent is actually granted: `agent_effective_grants` for each agent's grants, `grants_cover_server` for the `mcp:<slug>` match. `grants_cover_server` moves from `harness::mcp` (openhuman-gated) to `runtime::tools` beside `grant_matches` so the always-compiled console route reads the one primitive instead of reimplementing it; the registry path is unchanged. Overlay teammates are included deliberately — omitting them would let the zero-state falsely fire when an overlay agent reaches the server. Closes tinyhumansai#568 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughMCP server DTOs now include agent reachability derived from effective grants. Listing and mutation responses calculate reachability for manifest and overlay agents. The frontend displays authorized agents or a warning when no agent can access a server. ChangesMCP reachability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MCPServerOps
participant CompanyRecord
participant EffectiveAgentGrants
MCPServerOps->>CompanyRecord: Load company and roster
CompanyRecord->>EffectiveAgentGrants: Derive manifest and overlay grants
EffectiveAgentGrants-->>MCPServerOps: Return effective grants
MCPServerOps->>MCPServerOps: Calculate reachable agent IDs
MCPServerOps-->>MCPServerOps: Return server DTO with reachableBy
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…nsai#568) list_servers had reachability coverage but mutation_response did not — add a shape assertion on the PUT response in the delete-guard test so the field stays wired on the mutating path, not just on GET. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Conflict in src/server/ops/mcp.rs: main (tinyhumansai#598) renamed REBUILD_NOTE to NEXT_TURN_NOTE; this branch (tinyhumansai#568) added the reachable_by argument to dto_from_decl. Kept both.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/ops/mcp.rs`:
- Around line 294-300: Update reachers_of in src/server/ops/mcp.rs (lines
294-300) to accept the server declaration or enabled value and return an empty
list when the server is disabled, while preserving grant-based filtering for
enabled servers. Add or update the focused test in src/server/ops/write_test.rs
(lines 2545-2550) so a disabled server with matching grants asserts reachableBy:
[].
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f2543d1-f822-45c2-9ec1-c4a659d119a0
📒 Files selected for processing (6)
frontend/src/api/types.tsfrontend/src/views/connections/McpServersSection.tsxsrc/harness/mcp.rssrc/runtime/tools.rssrc/server/ops/mcp.rssrc/server/ops/write_test.rs
`registry_for_agent` filters on `decl.enabled && grants_cover_server(..)`, so an agent holding `mcp:<slug>` is handed no such tool while the server is off. `reachers_of` mirrored only the grant half, so a disabled server came back listing agents that cannot call it — the console/harness disagreement this feature exists to remove. It now takes the declaration and returns an empty list when the server is disabled. The console scopes its loud zero-state to enabled servers: a disabled server is empty by construction, so flagging it would cry wolf on intent. Test: a disabled server with an otherwise matching grant reports `reachableBy: []` in both readers — the mutating response that turns it off, and the later list.
oxoxDev
left a comment
There was a problem hiding this comment.
Relocating grants_cover_server from harness::mcp to runtime::tools rather than reimplementing a two-line check in the console route is the right call, and the reason is the one that matters: the console route ships in the default build and cannot import a #[cfg(feature = "openhuman")] symbol, so the alternative was a second matcher that could drift from the registry's. One matcher read by both is the whole point of #565, and a reachability display that can disagree with actual reachability would be worse than no display.
Including overlay teammates is also right, and the reasoning is the sharper half: iterating only manifest.agents would let the loud zero-state fire falsely when an overlay agent does reach the server — a console/harness disagreement announcing itself as a misconfiguration warning. Mirroring build_roster exactly, including the skip for an overlay id already claimed by a manifest agent, is what makes that hold.
Major — this collides with #554, on the two files where #554 owes frontend work.
This PR edits frontend/src/api/types.ts and frontend/src/views/connections/McpServersSection.tsx. Those are the exact two files #554 (feat(mcp): ship install-wide default MCP servers, @M3gA-Mind) has to touch: it adds a third McpSource::Default value to the DTO while types.ts:747 still declares source: "manifest" | "runtime", and McpServersSection.tsx:386 picks its badge variant with server.source === "manifest" ? … : …, so a default currently renders identically to a console-added server. I raised that on #554 as a blocker for its own stated rationale.
So both PRs are adding a field to the same interface and a new element to the same row. Neither is aware of the other. Concretely:
- If this lands first, #554's frontend fix-up rebases onto your
reachableBychanges — fine, but nobody has told @M3gA-Mind that. - If #554 lands first with the badge work, this rebases onto it — also fine, also untold.
- If #554 lands without the frontend work (its current state), you both end up editing
types.tsfor different reasons and thesourceunion stays wrong, with two PRs' worth of history making it look considered.
Worth a note on #554 agreeing an order, the way you did for #598.
src/server/ops/mcp.rs and src/server/ops/write_test.rs now have three claimants. #598 (approved, agreed to land first), #554, and this. #598 is small and green so it should still go first; after that these two need a sequence between themselves. Not a problem yet — it becomes one the moment two of them merge on the same afternoon.
Question — is the loud zero-state ever legitimate? "A healthy server no teammate can reach is almost always a misconfiguration" is right as a default. But a company mid-rollout — server added, grants not yet widened — hits it on purpose, and a warning that fires during a normal sequence teaches operators to ignore warnings. Is there a state where it reads as informational rather than wrong, or is the answer simply that the window is short enough not to matter? Worth a sentence either way, since the loudness is the feature.
Minor: reachableBy is computed by evaluating every agent's effective grants against every server on each list call. Almost certainly fine at company scale — flagging only so the cost is a known one rather than a discovered one, if a roster ever gets large.
Summary
Closes #568 (finding #3 of the MCP/Skills console↔harness epic #565).
The MCP console showed a server's health and tool count but never who could actually call it. An agent reaches
mcp:<slug>only when its effective grants cover it, so the moment a company narrows grants per agent, a server can be live, healthy, and reachable by nobody — silently, with no error anywhere in the chain (such an agent does not even getmcp_list_servers).GET .../mcp/servers(and every mutating response) now carriesreachableBy: the ids of the agents whose effective grants cover the server. The console renders it per row and flags the empty case loudly, because a healthy server no teammate can reach is almost always a misconfiguration rather than an intent.The design decision worth challenging
Reachability is computed over the same roster the harness builds — manifest agents plus promoted overlay teammates — using the exact machinery the harness registry uses, so the console cannot disagree with what an agent is actually granted (the thesis of #565):
agent_effective_grantsfor each agent's effective grants (already shared with the team-agent console route).grants_cover_serverfor themcp:<slug>match.grants_cover_servermoves fromharness::mcp(which is#[cfg(feature = "openhuman")]) toruntime::tools, besidegrant_matches. The console route ships in the default build and cannot import a harness symbol; relocating the primitive — rather than reimplementing the two-line check — keeps one matcher read by both the registry and the console (the "one validator" rule the codebase already follows). The registry path is unchanged.Overlay teammates are included, deliberately
build_rostergrants tools to manifest agents and overlay teammates (an overlay has notoolsrow, so it inherits the companyallow). Iterating onlymanifest.agentswould let the loud zero-state falsely fire when an overlay agent actually reaches the server — the exact console/harness disagreement #565 is about.roster_grantsmirrorsbuild_rosterexactly, including skipping an overlay id already claimed by a manifest agent.One note on the issue text
The issue says
agent_effective_grantsis "already shared between the harness and the console capability route." Accurate, but that console reader is the team-agent detail route, not an MCP route — this PR makes the MCP list route a new reader. No change to the approach; recording it for the next reader.API / behavior changes
McpServerDtogainsreachableBy: string[]— always serialized, even when empty (the empty array is the load-bearing signal). Additive; existing fields unchanged.registry_for_agentfilters ondecl.enabled && grants_cover_server(..), soreachers_ofmirrors both halves of that filter. The console scopes its loud zero-state to enabled servers — a disabled server is empty by construction, and flagging it would cry wolf on intent.list_serversandmutation_responseload the company record once and derive both the manifest servers and the roster from it — not an extra load.Tests
mcp_reachability_lists_reaching_agents_including_overlay— withallow = ["*"], a narrowed agent (tools = ["mcp:notion"]) reaches only that server, while a no-tools agent and an overlay teammate inherit the wildcard and reach everything.mcp_reachability_flags_a_server_no_agent_can_reach— a narrow companyallowleaves a manifest server with an emptyreachableBy(the flagged zero case).grants_cover_serverwith noopenhumanfeature.Verified locally:
cargo test --lib(default and--features openhuman,tinycortex),cargo clippy --no-deps --features openhuman,tinycortex --all-targets -- -D warnings,tsc -b --noEmit,cargo fmt --all -- --check— all clean.Depends on #554 — merge it first
#554 (
feat(mcp): install-wide default MCP servers) also editsMcpServerDto,list_servers, and the frontendMcpServertype, and changesresolve_effectivefrom 3 to 4 args. This PR is written againstmain; the agreed order (see #554/#598) is #554 → this. Once #554 merges I will follow up — adopting the 4-argresolve_effectiveand thesource: "default"servers, which flow through the reachability loop for free.mainhas since been merged in (not rebased, to keep the review history readable). The one conflict was insrc/server/ops/mcp.rs: #598 renamedREBUILD_NOTEtoNEXT_TURN_NOTEwhile this branch added thereachable_byargument todto_from_decl— both kept.Closes #568
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests