From b4a5fb3de6fd8ecfe88395b3a2457b94cb38048c Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 4 Jun 2025 00:58:07 +0000 Subject: [PATCH] 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 --- src/entrypoints/prepare.ts | 2 + src/mcp/install-mcp-server.ts | 93 +++++++++++++++++++++------------ test/install-mcp-server.test.ts | 77 ++++++++++++++++++++++----- 3 files changed, 126 insertions(+), 46 deletions(-) diff --git a/src/entrypoints/prepare.ts b/src/entrypoints/prepare.ts index 5736268..939f5bf 100644 --- a/src/entrypoints/prepare.ts +++ b/src/entrypoints/prepare.ts @@ -85,6 +85,7 @@ async function run() { // Step 11: Get MCP configuration const additionalMcpConfig = process.env.MCP_CONFIG || ""; + const allowedTools = process.env.ALLOWED_TOOLS || ""; const mcpConfig = await prepareMcpConfig({ githubToken, owner: context.repository.owner, @@ -92,6 +93,7 @@ async function run() { branch: branchInfo.currentBranch, additionalMcpConfig, claudeCommentId: commentId.toString(), + allowedTools, }); core.setOutput("mcp_config", mcpConfig); } catch (error) { diff --git a/src/mcp/install-mcp-server.ts b/src/mcp/install-mcp-server.ts index e820097..a83acbe 100644 --- a/src/mcp/install-mcp-server.ts +++ b/src/mcp/install-mcp-server.ts @@ -7,6 +7,7 @@ type PrepareConfigParams = { branch: string; additionalMcpConfig?: string; claudeCommentId?: string; + allowedTools?: string; }; export async function prepareMcpConfig( @@ -19,42 +20,68 @@ export async function prepareMcpConfig( branch, additionalMcpConfig, claudeCommentId, + allowedTools, } = params; try { + // Parse allowed tools into an array + const allowedToolsList = allowedTools + ? 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 = {}; + + // Only include github MCP server if GitHub tools are allowed + if (hasGitHubMcpTools) { + mcpServers.github = { + command: "docker", + args: [ + "run", + "-i", + "--rm", + "-e", + "GITHUB_PERSONAL_ACCESS_TOKEN", + "ghcr.io/github/github-mcp-server:sha-e9f748f", // https://github.com/github/github-mcp-server/releases/tag/v0.4.0 + ], + env: { + GITHUB_PERSONAL_ACCESS_TOKEN: githubToken, + }, + }; + } + + // 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", + args: [ + "run", + `${process.env.GITHUB_ACTION_PATH}/src/mcp/github-file-ops-server.ts`, + ], + env: { + GITHUB_TOKEN: githubToken, + REPO_OWNER: owner, + REPO_NAME: repo, + BRANCH_NAME: branch, + REPO_DIR: process.env.GITHUB_WORKSPACE || process.cwd(), + ...(claudeCommentId && { CLAUDE_COMMENT_ID: claudeCommentId }), + GITHUB_EVENT_NAME: process.env.GITHUB_EVENT_NAME || "", + IS_PR: process.env.IS_PR || "false", + }, + }; + } + const baseMcpConfig = { - mcpServers: { - github: { - command: "docker", - args: [ - "run", - "-i", - "--rm", - "-e", - "GITHUB_PERSONAL_ACCESS_TOKEN", - "ghcr.io/github/github-mcp-server:sha-e9f748f", // https://github.com/github/github-mcp-server/releases/tag/v0.4.0 - ], - env: { - GITHUB_PERSONAL_ACCESS_TOKEN: githubToken, - }, - }, - github_file_ops: { - command: "bun", - args: [ - "run", - `${process.env.GITHUB_ACTION_PATH}/src/mcp/github-file-ops-server.ts`, - ], - env: { - GITHUB_TOKEN: githubToken, - REPO_OWNER: owner, - REPO_NAME: repo, - BRANCH_NAME: branch, - REPO_DIR: process.env.GITHUB_WORKSPACE || process.cwd(), - ...(claudeCommentId && { CLAUDE_COMMENT_ID: claudeCommentId }), - GITHUB_EVENT_NAME: process.env.GITHUB_EVENT_NAME || "", - IS_PR: process.env.IS_PR || "false", - }, - }, - }, + mcpServers, }; // Merge with additional MCP config if provided diff --git a/test/install-mcp-server.test.ts b/test/install-mcp-server.test.ts index 3d2f02e..539c1fc 100644 --- a/test/install-mcp-server.test.ts +++ b/test/install-mcp-server.test.ts @@ -24,7 +24,7 @@ describe("prepareMcpConfig", () => { 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({ githubToken: "test-token", owner: "test-owner", @@ -34,11 +34,8 @@ describe("prepareMcpConfig", () => { const parsed = JSON.parse(result); 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.env.GITHUB_PERSONAL_ACCESS_TOKEN).toBe( - "test-token", - ); expect(parsed.mcpServers.github_file_ops.env.GITHUB_TOKEN).toBe( "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 () => { const result = await prepareMcpConfig({ githubToken: "test-token", @@ -60,7 +107,7 @@ describe("prepareMcpConfig", () => { const parsed = JSON.parse(result); expect(parsed.mcpServers).toBeDefined(); - expect(parsed.mcpServers.github).toBeDefined(); + expect(parsed.mcpServers.github).not.toBeDefined(); expect(parsed.mcpServers.github_file_ops).toBeDefined(); expect(consoleWarningSpy).not.toHaveBeenCalled(); }); @@ -76,7 +123,7 @@ describe("prepareMcpConfig", () => { const parsed = JSON.parse(result); expect(parsed.mcpServers).toBeDefined(); - expect(parsed.mcpServers.github).toBeDefined(); + expect(parsed.mcpServers.github).not.toBeDefined(); expect(parsed.mcpServers.github_file_ops).toBeDefined(); expect(consoleWarningSpy).not.toHaveBeenCalled(); }); @@ -100,6 +147,8 @@ describe("prepareMcpConfig", () => { repo: "test-repo", branch: "test-branch", additionalMcpConfig: additionalConfig, + allowedTools: + "mcp__github__create_issue,mcp__github_file_ops__commit_files", }); const parsed = JSON.parse(result); @@ -133,6 +182,8 @@ describe("prepareMcpConfig", () => { repo: "test-repo", branch: "test-branch", additionalMcpConfig: additionalConfig, + allowedTools: + "mcp__github__create_issue,mcp__github_file_ops__commit_files", }); const parsed = JSON.parse(result); @@ -174,7 +225,7 @@ describe("prepareMcpConfig", () => { const parsed = JSON.parse(result); expect(parsed.customProperty).toBe("custom-value"); expect(parsed.anotherProperty).toEqual({ nested: "value" }); - expect(parsed.mcpServers.github).toBeDefined(); + expect(parsed.mcpServers.github).not.toBeDefined(); expect(parsed.mcpServers.custom_server).toBeDefined(); }); @@ -193,7 +244,7 @@ describe("prepareMcpConfig", () => { expect(consoleWarningSpy).toHaveBeenCalledWith( 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(); }); @@ -215,7 +266,7 @@ describe("prepareMcpConfig", () => { expect(consoleWarningSpy).toHaveBeenCalledWith( 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(); }); @@ -237,7 +288,7 @@ describe("prepareMcpConfig", () => { expect(consoleWarningSpy).toHaveBeenCalledWith( 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(); }); @@ -258,7 +309,7 @@ describe("prepareMcpConfig", () => { expect(consoleInfoSpy).toHaveBeenCalledWith( "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(); // The array will be spread into the config (0: 1, 1: 2, 2: 3) expect(parsed[0]).toBe(1); @@ -300,7 +351,7 @@ describe("prepareMcpConfig", () => { const parsed = JSON.parse(result); expect(parsed.mcpServers.server1).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.env.CUSTOM).toBe("value"); expect(parsed.otherConfig.nested.deeply).toBe("value");