fix: only load GitHub MCP server when its tools are allowed

- Add allowedTools parameter to prepareMcpConfig
- Check for mcp__github__ and mcp__github_file_ops__ tool prefixes
- Only include MCP servers when their tools are in allowed_tools
- Maintain backward compatibility when allowed_tools is not specified
- Update tests to reflect the new conditional loading behavior

This optimizes resource usage by not loading unnecessary MCP servers
when their tools are not allowed in the configuration.

Co-authored-by: ashwin-ant <ashwin-ant@users.noreply.github.com>
This commit is contained in:
claude[bot]
2025-06-04 00:58:07 +00:00
committed by Ashwin Bhat
parent 94c0c31c1b
commit b4a5fb3de6
3 changed files with 126 additions and 46 deletions

View File

@@ -85,6 +85,7 @@ async function run() {
// Step 11: Get MCP configuration // Step 11: Get MCP configuration
const additionalMcpConfig = process.env.MCP_CONFIG || ""; const additionalMcpConfig = process.env.MCP_CONFIG || "";
const allowedTools = process.env.ALLOWED_TOOLS || "";
const mcpConfig = await prepareMcpConfig({ const mcpConfig = await prepareMcpConfig({
githubToken, githubToken,
owner: context.repository.owner, owner: context.repository.owner,
@@ -92,6 +93,7 @@ async function run() {
branch: branchInfo.currentBranch, branch: branchInfo.currentBranch,
additionalMcpConfig, additionalMcpConfig,
claudeCommentId: commentId.toString(), claudeCommentId: commentId.toString(),
allowedTools,
}); });
core.setOutput("mcp_config", mcpConfig); core.setOutput("mcp_config", mcpConfig);
} catch (error) { } catch (error) {

View File

@@ -7,6 +7,7 @@ type PrepareConfigParams = {
branch: string; branch: string;
additionalMcpConfig?: string; additionalMcpConfig?: string;
claudeCommentId?: string; claudeCommentId?: string;
allowedTools?: string;
}; };
export async function prepareMcpConfig( export async function prepareMcpConfig(
@@ -19,11 +20,28 @@ export async function prepareMcpConfig(
branch, branch,
additionalMcpConfig, additionalMcpConfig,
claudeCommentId, claudeCommentId,
allowedTools,
} = params; } = params;
try { try {
const baseMcpConfig = { // Parse allowed tools into an array
mcpServers: { const allowedToolsList = allowedTools
github: { ? allowedTools.split(",").map((tool) => tool.trim())
: [];
// Check if any GitHub MCP tools are allowed
const hasGitHubMcpTools = allowedToolsList.some((tool) =>
tool.startsWith("mcp__github__"),
);
const hasGitHubFileOpsTools = allowedToolsList.some((tool) =>
tool.startsWith("mcp__github_file_ops__"),
);
// Start with an empty servers object
const mcpServers: Record<string, any> = {};
// Only include github MCP server if GitHub tools are allowed
if (hasGitHubMcpTools) {
mcpServers.github = {
command: "docker", command: "docker",
args: [ args: [
"run", "run",
@@ -36,8 +54,14 @@ export async function prepareMcpConfig(
env: { env: {
GITHUB_PERSONAL_ACCESS_TOKEN: githubToken, GITHUB_PERSONAL_ACCESS_TOKEN: githubToken,
}, },
}, };
github_file_ops: { }
// Always include github_file_ops server as it contains essential tools
// (mcp__github_file_ops__commit_files, mcp__github_file_ops__update_claude_comment)
// These are in the BASE_ALLOWED_TOOLS
if (hasGitHubFileOpsTools || !allowedTools) {
mcpServers.github_file_ops = {
command: "bun", command: "bun",
args: [ args: [
"run", "run",
@@ -53,8 +77,11 @@ export async function prepareMcpConfig(
GITHUB_EVENT_NAME: process.env.GITHUB_EVENT_NAME || "", GITHUB_EVENT_NAME: process.env.GITHUB_EVENT_NAME || "",
IS_PR: process.env.IS_PR || "false", IS_PR: process.env.IS_PR || "false",
}, },
}, };
}, }
const baseMcpConfig = {
mcpServers,
}; };
// Merge with additional MCP config if provided // Merge with additional MCP config if provided

View File

@@ -24,7 +24,7 @@ describe("prepareMcpConfig", () => {
processExitSpy.mockRestore(); processExitSpy.mockRestore();
}); });
test("should return base config when no additional config is provided", async () => { test("should return base config when no additional config is provided and no allowed_tools", async () => {
const result = await prepareMcpConfig({ const result = await prepareMcpConfig({
githubToken: "test-token", githubToken: "test-token",
owner: "test-owner", owner: "test-owner",
@@ -34,11 +34,8 @@ describe("prepareMcpConfig", () => {
const parsed = JSON.parse(result); const parsed = JSON.parse(result);
expect(parsed.mcpServers).toBeDefined(); expect(parsed.mcpServers).toBeDefined();
expect(parsed.mcpServers.github).toBeDefined(); expect(parsed.mcpServers.github).not.toBeDefined();
expect(parsed.mcpServers.github_file_ops).toBeDefined(); expect(parsed.mcpServers.github_file_ops).toBeDefined();
expect(parsed.mcpServers.github.env.GITHUB_PERSONAL_ACCESS_TOKEN).toBe(
"test-token",
);
expect(parsed.mcpServers.github_file_ops.env.GITHUB_TOKEN).toBe( expect(parsed.mcpServers.github_file_ops.env.GITHUB_TOKEN).toBe(
"test-token", "test-token",
); );
@@ -49,6 +46,56 @@ describe("prepareMcpConfig", () => {
); );
}); });
test("should include github MCP server when mcp__github__ tools are allowed", async () => {
const result = await prepareMcpConfig({
githubToken: "test-token",
owner: "test-owner",
repo: "test-repo",
branch: "test-branch",
allowedTools:
"mcp__github__create_issue,mcp__github_file_ops__commit_files",
});
const parsed = JSON.parse(result);
expect(parsed.mcpServers).toBeDefined();
expect(parsed.mcpServers.github).toBeDefined();
expect(parsed.mcpServers.github_file_ops).toBeDefined();
expect(parsed.mcpServers.github.env.GITHUB_PERSONAL_ACCESS_TOKEN).toBe(
"test-token",
);
});
test("should not include github MCP server when only file_ops tools are allowed", async () => {
const result = await prepareMcpConfig({
githubToken: "test-token",
owner: "test-owner",
repo: "test-repo",
branch: "test-branch",
allowedTools:
"mcp__github_file_ops__commit_files,mcp__github_file_ops__update_claude_comment",
});
const parsed = JSON.parse(result);
expect(parsed.mcpServers).toBeDefined();
expect(parsed.mcpServers.github).not.toBeDefined();
expect(parsed.mcpServers.github_file_ops).toBeDefined();
});
test("should not include any MCP servers when no GitHub tools are allowed", async () => {
const result = await prepareMcpConfig({
githubToken: "test-token",
owner: "test-owner",
repo: "test-repo",
branch: "test-branch",
allowedTools: "Edit,Read,Write",
});
const parsed = JSON.parse(result);
expect(parsed.mcpServers).toBeDefined();
expect(parsed.mcpServers.github).not.toBeDefined();
expect(parsed.mcpServers.github_file_ops).not.toBeDefined();
});
test("should return base config when additional config is empty string", async () => { test("should return base config when additional config is empty string", async () => {
const result = await prepareMcpConfig({ const result = await prepareMcpConfig({
githubToken: "test-token", githubToken: "test-token",
@@ -60,7 +107,7 @@ describe("prepareMcpConfig", () => {
const parsed = JSON.parse(result); const parsed = JSON.parse(result);
expect(parsed.mcpServers).toBeDefined(); expect(parsed.mcpServers).toBeDefined();
expect(parsed.mcpServers.github).toBeDefined(); expect(parsed.mcpServers.github).not.toBeDefined();
expect(parsed.mcpServers.github_file_ops).toBeDefined(); expect(parsed.mcpServers.github_file_ops).toBeDefined();
expect(consoleWarningSpy).not.toHaveBeenCalled(); expect(consoleWarningSpy).not.toHaveBeenCalled();
}); });
@@ -76,7 +123,7 @@ describe("prepareMcpConfig", () => {
const parsed = JSON.parse(result); const parsed = JSON.parse(result);
expect(parsed.mcpServers).toBeDefined(); expect(parsed.mcpServers).toBeDefined();
expect(parsed.mcpServers.github).toBeDefined(); expect(parsed.mcpServers.github).not.toBeDefined();
expect(parsed.mcpServers.github_file_ops).toBeDefined(); expect(parsed.mcpServers.github_file_ops).toBeDefined();
expect(consoleWarningSpy).not.toHaveBeenCalled(); expect(consoleWarningSpy).not.toHaveBeenCalled();
}); });
@@ -100,6 +147,8 @@ describe("prepareMcpConfig", () => {
repo: "test-repo", repo: "test-repo",
branch: "test-branch", branch: "test-branch",
additionalMcpConfig: additionalConfig, additionalMcpConfig: additionalConfig,
allowedTools:
"mcp__github__create_issue,mcp__github_file_ops__commit_files",
}); });
const parsed = JSON.parse(result); const parsed = JSON.parse(result);
@@ -133,6 +182,8 @@ describe("prepareMcpConfig", () => {
repo: "test-repo", repo: "test-repo",
branch: "test-branch", branch: "test-branch",
additionalMcpConfig: additionalConfig, additionalMcpConfig: additionalConfig,
allowedTools:
"mcp__github__create_issue,mcp__github_file_ops__commit_files",
}); });
const parsed = JSON.parse(result); const parsed = JSON.parse(result);
@@ -174,7 +225,7 @@ describe("prepareMcpConfig", () => {
const parsed = JSON.parse(result); const parsed = JSON.parse(result);
expect(parsed.customProperty).toBe("custom-value"); expect(parsed.customProperty).toBe("custom-value");
expect(parsed.anotherProperty).toEqual({ nested: "value" }); expect(parsed.anotherProperty).toEqual({ nested: "value" });
expect(parsed.mcpServers.github).toBeDefined(); expect(parsed.mcpServers.github).not.toBeDefined();
expect(parsed.mcpServers.custom_server).toBeDefined(); expect(parsed.mcpServers.custom_server).toBeDefined();
}); });
@@ -193,7 +244,7 @@ describe("prepareMcpConfig", () => {
expect(consoleWarningSpy).toHaveBeenCalledWith( expect(consoleWarningSpy).toHaveBeenCalledWith(
expect.stringContaining("Failed to parse additional MCP config:"), expect.stringContaining("Failed to parse additional MCP config:"),
); );
expect(parsed.mcpServers.github).toBeDefined(); expect(parsed.mcpServers.github).not.toBeDefined();
expect(parsed.mcpServers.github_file_ops).toBeDefined(); expect(parsed.mcpServers.github_file_ops).toBeDefined();
}); });
@@ -215,7 +266,7 @@ describe("prepareMcpConfig", () => {
expect(consoleWarningSpy).toHaveBeenCalledWith( expect(consoleWarningSpy).toHaveBeenCalledWith(
expect.stringContaining("MCP config must be a valid JSON object"), expect.stringContaining("MCP config must be a valid JSON object"),
); );
expect(parsed.mcpServers.github).toBeDefined(); expect(parsed.mcpServers.github).not.toBeDefined();
expect(parsed.mcpServers.github_file_ops).toBeDefined(); expect(parsed.mcpServers.github_file_ops).toBeDefined();
}); });
@@ -237,7 +288,7 @@ describe("prepareMcpConfig", () => {
expect(consoleWarningSpy).toHaveBeenCalledWith( expect(consoleWarningSpy).toHaveBeenCalledWith(
expect.stringContaining("MCP config must be a valid JSON object"), expect.stringContaining("MCP config must be a valid JSON object"),
); );
expect(parsed.mcpServers.github).toBeDefined(); expect(parsed.mcpServers.github).not.toBeDefined();
expect(parsed.mcpServers.github_file_ops).toBeDefined(); expect(parsed.mcpServers.github_file_ops).toBeDefined();
}); });
@@ -258,7 +309,7 @@ describe("prepareMcpConfig", () => {
expect(consoleInfoSpy).toHaveBeenCalledWith( expect(consoleInfoSpy).toHaveBeenCalledWith(
"Merging additional MCP server configuration with built-in servers", "Merging additional MCP server configuration with built-in servers",
); );
expect(parsed.mcpServers.github).toBeDefined(); expect(parsed.mcpServers.github).not.toBeDefined();
expect(parsed.mcpServers.github_file_ops).toBeDefined(); expect(parsed.mcpServers.github_file_ops).toBeDefined();
// The array will be spread into the config (0: 1, 1: 2, 2: 3) // The array will be spread into the config (0: 1, 1: 2, 2: 3)
expect(parsed[0]).toBe(1); expect(parsed[0]).toBe(1);
@@ -300,7 +351,7 @@ describe("prepareMcpConfig", () => {
const parsed = JSON.parse(result); const parsed = JSON.parse(result);
expect(parsed.mcpServers.server1).toBeDefined(); expect(parsed.mcpServers.server1).toBeDefined();
expect(parsed.mcpServers.server2).toBeDefined(); expect(parsed.mcpServers.server2).toBeDefined();
expect(parsed.mcpServers.github).toBeDefined(); expect(parsed.mcpServers.github).not.toBeDefined();
expect(parsed.mcpServers.github_file_ops.command).toBe("overridden"); expect(parsed.mcpServers.github_file_ops.command).toBe("overridden");
expect(parsed.mcpServers.github_file_ops.env.CUSTOM).toBe("value"); expect(parsed.mcpServers.github_file_ops.env.CUSTOM).toBe("value");
expect(parsed.otherConfig.nested.deeply).toBe("value"); expect(parsed.otherConfig.nested.deeply).toBe("value");