feat(agent): attach MCP servers to agent runs, gated by issue source - #131
Open
ArnabChatterjee20k wants to merge 8 commits into
Open
feat(agent): attach MCP servers to agent runs, gated by issue source#131ArnabChatterjee20k wants to merge 8 commits into
ArnabChatterjee20k wants to merge 8 commits into
Conversation
Adds config-driven MCP support so the Claude agent can query the
production Appwrite Cloud (via the Appwrite MCP server) instead of a
local stack when investigating user-reported issues.
- New McpServerConfig under ProviderConfig.mcp, keyed by server name,
with a per-server sources list (default use: helpscout only; empty
means all sources).
- Runner renders matched servers to a private 0600 .mcp.json temp file,
passes --mcp-config/--strict-mcp-config, and auto-allowlists
mcp__<server> tools for both fix and Q&A runs. Secrets stay in env
via ${VAR} expansion; runs without an issue never attach MCP.
- Threads issue source through the execute path; wires mcp through both
runner build sites (lib.rs, main.rs).
- Example config + unit tests (round-trip, source gating, rendered
config perms/shape).
There was a problem hiding this comment.
Pull request overview
This PR adds config-driven MCP server support to Claude agent runs, so investigations can query production services via MCP (e.g., Appwrite) only when the run is tied to an issue and the issue’s source is allowlisted for that server.
Changes:
- Introduces
McpServerConfigandProviderConfig.mcp(keyed by server name) with per-server source gating and TOML parsing/tests. - Threads MCP config through provider runner construction and updates the Claude runner to render a per-run temp MCP config and pass it to the Claude CLI.
- Adds example configuration and unit tests for MCP config rendering and source matching.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Plumbs provider mcp config into the runtime runner config. |
| src/lib.rs | Ensures build_provider_runner carries mcp into ClaudeRunnerConfig. |
| crates/claudear-integrations/src/runner/claude.rs | Implements MCP server matching/rendering and attaches MCP config + tool allowlisting per run; adds tests. |
| crates/claudear-integrations/Cargo.toml | Adds tempfile dependency for rendered MCP config files. |
| crates/claudear-config/src/config.rs | Adds McpServerConfig, ProviderConfig.mcp, source gating logic, and tests. |
| claudear.example.toml | Documents example MCP server configuration and source gating. |
Suppressed comments (1)
crates/claudear-integrations/src/runner/claude.rs:766
execute_with_env_and_attemptexpectssource: Option<&str>, but this passesSome(&issue.source)(typeOption<&String>), which will not compile. Pass a&strinstead (e.g.issue.source.as_str()).
.execute_with_env_and_attempt(
&prompt,
&issue.short_id,
env,
None,
project_dir,
false,
Some(&issue.source),
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThe PR adds source-gated, configuration-driven MCP server attachment to Claude agent runs.
Confidence Score: 4/5The PR does not yet appear safe to merge because read-only runs can receive mutating MCP permissions and unrestricted server permissions can still fail to authorize namespaced tools. Source-matched MCP permissions are appended uniformly to fix and read-only verification or reply runs, preserving the previously reported capability-boundary violation. The empty-tools path also still emits only the bare server permission rather than permissions matching the server's namespaced tools. Files Needing Attention: crates/claudear-integrations/src/runner/claude.rs Important Files Changed
Reviews (7): Last reviewed commit: "refactor(agent): single tools allowlist ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Pass issue source as &str via as_str() in verify/reply paths. - Write rendered MCP config to the open temp handle instead of reopening the path (avoids Windows exclusive-lock failures). - Validate exactly one of command/url per server; skip and warn otherwise so strict MCP loading never sees an ambiguous transport. - Add per-server tools allowlist: scope to mcp__<server>__<tool> when set, else grant all via mcp__<server>. Lets read-only runs be scoped to read tools; read-only API key remains the enforced boundary. - Example config shows read-only tool scoping; config test covers tools.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/claudear-integrations/src/runner/claude.rs:781
- The doc comment says this renders into a
.mcp.jsonfile with 0600 permissions, but the implementation writes a temp file namedclaudear-mcp-*.json, and 0600 is only meaningful/guaranteed on Unix. Clarifying the wording avoids misleading readers about the on-disk filename and portability.
.error
.unwrap_or_else(|| "Failed to generate reply".to_string()),
))
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
crates/claudear-integrations/src/runner/claude.rs:786
- The doc comment says this renders a “.mcp.json”, but the tempfile is created with a generic
.jsonsuffix and is passed explicitly via--mcp-config. Updating the comment avoids confusion when debugging or searching for the file.
/// Render matched MCP servers into a `.mcp.json` in a private temp file (0600),
/// deleted when the returned handle drops. `${VAR}` in env is expanded by the CLI.
Comment on lines
+920
to
+939
| // Tools to allowlist for the attached servers (empty when none). A server | ||
| // with an explicit `tools` list is scoped to `mcp__<server>__<tool>`; | ||
| // otherwise all of its tools are granted via `mcp__<server>`. | ||
| let mcp_tool_globs: Vec<String> = if mcp_config_file.is_some() { | ||
| matched_mcp | ||
| .iter() | ||
| .flat_map(|(name, cfg)| { | ||
| if cfg.tools.is_empty() { | ||
| vec![format!("mcp__{}", name)] | ||
| } else { | ||
| cfg.tools | ||
| .iter() | ||
| .map(|tool| format!("mcp__{}__{}", name, tool)) | ||
| .collect() | ||
| } | ||
| }) | ||
| .collect() | ||
| } else { | ||
| Vec::new() | ||
| }; |
- Read-only runs (Q&A/verify/reply) no longer get unscoped MCP tools; a server must declare an explicit `tools` allowlist to be usable there. Fix runs still default to all tools. Closes the read-only-boundary gap. - Validate transport consistency: reject `command` with a non-stdio type and `url` with stdio, not just presence, so --strict-mcp-config never sees a contradictory server. Added has_valid_transport() + tests. - Clarify render_mcp_config doc (temp filename, 0600 is Unix-only).
Read-only runs (Q&A/verify/reply) now draw tools only from a dedicated per-server readonly_tools list, never from `tools` (which may include mutating tools used by fix runs). Since a tool's capability cannot be verified at config time, the operator must explicitly list non-mutating tools for read-only use; with none listed, read-only runs get no MCP tools. Closes the remaining production-mutation path.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/claudear-integrations/src/runner/claude.rs:808
- The HTTP/SSE transport branch in
render_mcp_config(whencfg.urlis set) is currently untested; the only added unit test covers the stdio/commandpath. Adding a test that asserts the rendered JSON includes the correcttype/url/headers(and does not include stdio-only fields likecommand/args/env) would help prevent regressions.
} else if let Some(ref url) = cfg.url {
// http/sse transport
entry.insert(
"type".to_string(),
json!(cfg.transport.clone().unwrap_or_else(|| "http".to_string())),
Asserts the url branch emits type/url/headers and omits stdio-only fields (command/args/env). Addresses review coverage gap.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
crates/claudear-integrations/src/runner/claude.rs:3845
- Same as the stdio test: reopening the
NamedTempFileby path can be brittle on Windows. Prefer seeking and parsing from the already-open handle.
let file = ClaudeAgentRunner::render_mcp_config(&servers).expect("render");
let doc: serde_json::Value =
serde_json::from_str(&std::fs::read_to_string(file.path()).unwrap()).unwrap();
crates/claudear-integrations/src/runner/claude.rs:963
- The comment says this always ignores any repo
.mcp.json, but the CLI flags are only added whenmcp_config_fileis set. Either clarify the comment to reflect the conditional behavior, or always pass an explicit (possibly empty) MCP config when you need to enforce source-gated attachment.
// Load only our rendered MCP config, ignoring any repo .mcp.json.
crates/claudear-integrations/src/runner/claude.rs:3817
- These tests reopen the temp file via
std::fs::read_to_string(file.path()), butrender_mcp_configexplicitly avoids reopening handles due to Windows locking behavior. To keep the tests robust across platforms, read via the already-open handle (seek back to start) and parse withserde_json::from_reader.
This issue also appears on line 3843 of the same file.
let file = ClaudeAgentRunner::render_mcp_config(&servers).expect("render");
let doc: serde_json::Value =
serde_json::from_str(&std::fs::read_to_string(file.path()).unwrap()).unwrap();
- Reword the strict-mcp-config comment: flags are only added when a config is attached; no MCP flags when nothing matches. - Render tests read the temp file via reopen() instead of by path, matching render_mcp_config's Windows-safe handle write.
The API key is the access boundary, so a separate read-only tool list added complexity without a real guarantee. Collapse to one `tools` array (empty = all of the server's tools) applied uniformly to fix and Q&A runs. Drop readonly_tools.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds config-driven MCP support so the Claude agent can query the production Appwrite Cloud (via the Appwrite MCP server) instead of a local stack when investigating user-reported issues.
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)