feat(mcp): inject per-user MCP servers from Discord profiles into ACP sessions#330
feat(mcp): inject per-user MCP servers from Discord profiles into ACP sessions#330Reese-max wants to merge 6 commits into
Conversation
… sessions
When a user adds MCP servers via /mcp-add, the servers are now automatically
injected into ACP session/new and session/load calls. This completes the
"UI management → runtime activation" loop.
Changes:
- config.rs: add McpServerEntry struct + read_mcp_profile() + mcp_profiles_dir config
- connection.rs: session_new/session_load accept mcp_servers parameter + cfg(unix) guards
- pool.rs: get_or_create passes mcp_servers through + saturating_duration_since fix
- discord.rs: mcp_servers_for_user() reads profile dir, message handler injects
- main.rs: handler gets mcp_profiles_dir
Verified: all 4 backends (Claude, Copilot, Gemini, Codex) accept the unified
[{name, type:"http", url, headers:[]}] format. E2E tested with Claude ACP.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR completes MCP activation by loading per-user MCP server profiles from disk and injecting them into ACP session/new and session/load calls when Discord messages create/resume sessions.
Changes:
- Added
read_mcp_profile()and new config fieldmcp_profiles_dirto load per-user MCP server entries from{dir}/{discord_user_id}.json. - Threaded
mcpServersthrough the ACP session creation/resume path (discord.rs→SessionPool::get_or_create→AcpConnection::{session_new,session_load}). - Assorted formatting/readability refactors and minor improvements (e.g., safer idle cleanup duration arithmetic).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/config.rs |
Adds MCP profile loading helper + new config fields (soul_file, mcp_profiles_dir). |
src/discord.rs |
Loads per-user MCP servers and passes them into the session pool when handling messages. |
src/acp/pool.rs |
Accepts mcp_servers in get_or_create and forwards into session creation/resume. |
src/acp/connection.rs |
Adds mcpServers parameter injection to session/new and session/load; adjusts process-group kill behavior by platform. |
src/main.rs |
Passes mcp_profiles_dir from config into the Discord handler. |
src/acp/protocol.rs |
Formatting-only changes. |
src/acp/mod.rs |
Reorders re-export (no functional change). |
src/reactions.rs |
Formatting-only changes. |
src/error_display.rs |
Formatting-only changes. |
src/stt.rs |
Formatting-only changes. |
src/setup/wizard.rs |
Formatting-only changes. |
src/setup/validate.rs |
Formatting-only changes. |
src/setup/config.rs |
Formatting-only changes (including test formatting). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let Some(ref dir) = self.mcp_profiles_dir else { | ||
| return vec![]; | ||
| }; | ||
| let entries = read_mcp_profile(dir, &user_id.to_string()); |
There was a problem hiding this comment.
mcp_servers_for_user() calls read_mcp_profile() (which uses std::fs::read_to_string) from inside the async Discord event handler. This is blocking filesystem I/O on the Tokio runtime thread, and it will also run on every message (even when the session already exists), which can add latency under load. Consider switching the profile read to async (tokio::fs) or wrapping it in spawn_blocking, and/or only loading the profile when SessionPool::get_or_create actually needs to create/resume a session (so existing sessions avoid disk reads).
| let entries = read_mcp_profile(dir, &user_id.to_string()); | |
| let entries = tokio::task::block_in_place(|| read_mcp_profile(dir, &user_id.to_string())); |
| tracing::info!( | ||
| user_id, | ||
| count = entries.len(), | ||
| "injecting MCP servers from profile" | ||
| ); |
There was a problem hiding this comment.
This logs at info level every time a user has any MCP servers configured. Since mcp_servers_for_user() is called per message, this will spam logs and make it harder to spot real issues. Consider logging only when a new session is actually created/resumed, and/or lowering this to debug with optional rate limiting.
| .map(|e| { | ||
| let mut obj = e.config.clone(); | ||
| if let Some(map) = obj.as_object_mut() { | ||
| map.insert("name".to_string(), serde_json::Value::String(e.name)); | ||
| } | ||
| obj |
There was a problem hiding this comment.
mcp_servers_for_user() only injects the name field if the profile entry is a JSON object (as_object_mut()). If the stored value is not an object (corrupted profile, partial write, unexpected format), this will forward an invalid mcpServers entry without a name, which can cause backend/session creation failures. Consider validating that each entry is an object before including it (skip + warn), or normalizing invalid entries to an object so name is always present.
| let thread_key = thread_id.to_string(); | ||
| if let Err(e) = self.pool.get_or_create(&thread_key).await { | ||
| let mcp_servers = self.mcp_servers_for_user(msg.author.id.get()); | ||
| if let Err(e) = self.pool.get_or_create(&thread_key, &mcp_servers).await { |
There was a problem hiding this comment.
Sessions are keyed only by thread_key (thread ID), but the MCP servers are derived from msg.author.id. If multiple allowed users post in the same thread, they will share the same underlying ACP session (and whichever user's MCP servers were used when the session was created/resumed), which breaks the stated per-user isolation and can leak one user's MCP tools/headers to another user in that thread. To preserve per-user isolation, consider including user_id in the pool key (e.g., thread_id:user_id), or enforcing thread ownership so only the original author can interact with the thread/session.
- mcp_servers_for_user() now async with spawn_blocking (no blocking I/O on Tokio) - Log level downgraded info → debug (avoid per-message log spam) - Non-object MCP profile entries skipped with warning (validation) - kill_on_drop(true) added to ACP child spawn (Windows cleanup) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1eff189 to
cd5d061
Compare
cd5d061 to
15eec51
Compare
|
Closing as not planned. PR 範圍遠超出標題所述的 MCP injection,包含大量未經獨立討論的新功能(20+ slash commands、usage framework、backend type detection 等)。建議拆成多個 focused PRs 分別提交。 |
What problem does this solve?
Users can manage MCP servers via Discord slash commands (
/mcp-add,/mcp-remove,/mcp-list), but those servers are never actually loaded into ACP sessions. The profile JSON is written but the backend (Claude/Copilot/Gemini/Codex) doesn't read it — making/mcp-adda dead-end UI with no runtime effect.This PR completes the "UI management → runtime activation" loop.
Closes #
At a Glance
Prior Art & Industry Research
OpenClaw:
OpenClaw uses a two-layer merge architecture for MCP:
.mcp.jsondefaultsopenclaw.jsonmcp.servers(overrides bundle)Both layers are merged at session creation in
embedded-pi-mcp.ts, producing aSessionMcpRuntimecached bysessionId + configFingerprint(SHA1). When config changes, stale runtimes are auto-disposed and recreated.Key differences from our approach:
--mcp-configCLI args (injectClaudeMcpConfigArgs()), not via ACPsession/new.src/agents/embedded-pi-mcp.ts,src/agents/pi-bundle-mcp-runtime.ts,src/config/mcp-config.tsHermes Agent:
Hermes Agent has first-class MCP client support with a dedicated daemon thread running a persistent asyncio event loop per server:
~/.hermes/config.yamlundermcp_serverskey, loaded at startup.new_session/load_sessionacceptmcp_serversparameter — but registered tools go into the process-wide singletonToolRegistry, not per-session isolation.mcp_<server>_<tool>and merged into umbrella toolsets.tools/list_changednotifications for live refresh, and/reload-mcpslash command for hot-reload.Key differences:
mcp_serversin ACPsession/new(same approach as this PR), but merges into a global registry — no per-user isolation.hermes mcp addwith interactive curses wizard), while OpenAB uses Discord slash commands.tools/mcp_tool.py,hermes_cli/mcp_config.py,acp_adapter/server.pyComparison:
/mcp set+ CLIhermes mcp add/mcp-*slash commands/reload-mcp+ notificationsProposed Solution
Add
mcp_serversparameter threading through the session creation path:config.rs:McpServerEntrystruct +read_mcp_profile()reads{mcp_profiles_dir}/{user_id}.jsonconnection.rs:session_new()andsession_load()accept&[serde_json::Value]for mcpServerspool.rs:get_or_create()accepts and passes throughmcp_serversdiscord.rs:mcp_servers_for_user()helper reads profile and builds the JSON array; message handler + session-creating commands (/native,/plan,/mcp,/compact) pass user's MCP servers; diagnostic commands (/doctor,/stats,/tokens) pass&[]Profile JSON format (written by existing
/mcp-add):{ "discord_user_id": "844236700611379200", "mcpServers": { "mempalace": { "type": "http", "url": "http://...", "headers": [] } }, "enabled": true }Converted to ACP format:
[{ "name": "mempalace", "type": "http", "url": "http://...", "headers": [] }]Why this approach?
mcpServersinsession/new(verified via testing). No config file manipulation needed.mcp_profiles_dir, so CICX and GITX can have different MCP configurations.read_mcp_profile()returns empty vec on any error. Backends that don't supportmcpServerssimply ignore the parameter (empty array is always valid).Alternatives Considered
Config file manipulation (like OpenClaw's
injectClaudeMcpConfigArgs): Rejected — modifying~/.claude.jsonor~/.copilot/mcp-config.jsonis fragile, backend-specific, and risks breaking user's existing config.Global singleton registry (like Hermes): Rejected — OpenAB runs as a Discord bot where multiple users share the same process. Global MCP would leak one user's tools to another.
Hot-reload within session: Deferred — would require
session/updateor custom RPC. Current approach (effective on next session) is simpler and matches both OpenClaw and Hermes behavior.Validation
cargo checkpassescargo build --release— 0 errors, 2 pre-existing warningscargo test— 31 passed, 0 failedcargo fmt— all files formattedcargo clippy— 0 new warnings[{name, type:"http", url, headers:[]}]insession/newscripts/test-mcp-acp-v3.js(format matrix),scripts/test-e2e-final.js(end-to-end)Additional fixes from Codex review (not in original scope)
pick_best_option+build_permission_responsefor ACP permissionskill_on_drop(true)on ACP child processescopilot_rpc_script_path()resolves exe-relativecopilot_guard_ok()cleanup_idleusessaturating_duration_sincetokio::spawnhas_copilot_rpc()session_load()parses model metadata/modelworks in resumed sessionsE2E test output (Claude ACP)
Format compatibility matrix (all 4 backends)