anthropics/claude-code-action
1
0
Fork 0
mirror of https://github.com/anthropics/claude-code-action.git synced 2026-01-22 22:44:13 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: only load GitHub MCP server when its tools are allowed (#124)

Browse Source 
* 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>

* fix: always load github_file_ops server regardless of allowed_tools

- Only apply conditional loading to the github MCP server
- Always load github_file_ops server as it contains essential tools
- Update tests to reflect this behavior

Co-authored-by: ashwin-ant <ashwin-ant@users.noreply.github.com>

* refactor: move allowedTools/disallowedTools parsing to parseGitHubContext

- Change allowedTools and disallowedTools from string to string[] in ParsedGitHubContext type
- Parse comma-separated environment variables into arrays in parseGitHubContext function
- Update create-prompt and install-mcp-server to use pre-parsed arrays
- Update all affected test files to use array syntax
- Eliminate duplicate parsing logic across the codebase

* style: apply prettier formatting

---------

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: ashwin-ant <ashwin-ant@users.noreply.github.com>
This commit is contained in:
Ashwin Bhat
2025-06-04 11:56:56 -07:00
committed by GitHub
parent 94c0c31c1b
commit 699aa26b41
10 changed files with 159 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
test/create-prompt.test.ts
View File

@@ -652,7 +652,7 @@ describe("buildAllowedToolsString", () => {
});
test("should append custom tools when provided", () => {
const customTools = "Tool1,Tool2,Tool3";
const customTools = ["Tool1", "Tool2", "Tool3"];
const result = buildAllowedToolsString(customTools);
// Base tools should be present
@@ -683,7 +683,7 @@ describe("buildDisallowedToolsString", () => {
});
test("should append custom disallowed tools when provided", () => {
const customDisallowedTools = "BadTool1,BadTool2";
const customDisallowedTools = ["BadTool1", "BadTool2"];
const result = buildDisallowedToolsString(customDisallowedTools);
// Base disallowed tools should be present
@@ -701,8 +701,8 @@ describe("buildDisallowedToolsString", () => {
});
test("should remove hardcoded disallowed tools if they are in allowed tools", () => {
const customDisallowedTools = "BadTool1,BadTool2";
const allowedTools = "WebSearch,SomeOtherTool";
const customDisallowedTools = ["BadTool1", "BadTool2"];
const allowedTools = ["WebSearch", "SomeOtherTool"];
const result = buildDisallowedToolsString(
customDisallowedTools,
allowedTools,
@@ -720,7 +720,7 @@ describe("buildDisallowedToolsString", () => {
});
test("should remove all hardcoded disallowed tools if they are all in allowed tools", () => {
const allowedTools = "WebSearch,WebFetch,SomeOtherTool";
const allowedTools = ["WebSearch", "WebFetch", "SomeOtherTool"];
const result = buildDisallowedToolsString(undefined, allowedTools);
// Both hardcoded disallowed tools should be removed
@@ -732,8 +732,8 @@ describe("buildDisallowedToolsString", () => {
});
test("should handle custom disallowed tools when all hardcoded tools are overridden", () => {
const customDisallowedTools = "BadTool1,BadTool2";
const allowedTools = "WebSearch,WebFetch";
const customDisallowedTools = ["BadTool1", "BadTool2"];
const allowedTools = ["WebSearch", "WebFetch"];
const result = buildDisallowedToolsString(
customDisallowedTools,
allowedTools,

96
test/install-mcp-server.test.ts
View File

@@ -24,21 +24,19 @@ 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",
repo: "test-repo",
branch: "test-branch",
allowedTools: [],
});
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 +47,60 @@ 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 include file_ops server even 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).toBeDefined();
});
test("should return base config when additional config is empty string", async () => {
const result = await prepareMcpConfig({
githubToken: "test-token",
@@ -56,11 +108,12 @@ describe("prepareMcpConfig", () => {
repo: "test-repo",
branch: "test-branch",
additionalMcpConfig: "",
allowedTools: [],
});
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();
});
@@ -72,11 +125,12 @@ describe("prepareMcpConfig", () => {
repo: "test-repo",
branch: "test-branch",
additionalMcpConfig: " \n\t ",
allowedTools: [],
});
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 +154,10 @@ 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 +191,10 @@ 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);
@@ -169,12 +231,13 @@ describe("prepareMcpConfig", () => {
repo: "test-repo",
branch: "test-branch",
additionalMcpConfig: additionalConfig,
allowedTools: [],
});
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();
});
@@ -187,13 +250,14 @@ describe("prepareMcpConfig", () => {
repo: "test-repo",
branch: "test-branch",
additionalMcpConfig: invalidJson,
allowedTools: [],
});
const parsed = JSON.parse(result);
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();
});
@@ -206,6 +270,7 @@ describe("prepareMcpConfig", () => {
repo: "test-repo",
branch: "test-branch",
additionalMcpConfig: nonObjectJson,
allowedTools: [],
});
const parsed = JSON.parse(result);
@@ -215,7 +280,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();
});
@@ -228,6 +293,7 @@ describe("prepareMcpConfig", () => {
repo: "test-repo",
branch: "test-branch",
additionalMcpConfig: nullJson,
allowedTools: [],
});
const parsed = JSON.parse(result);
@@ -237,7 +303,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();
});
@@ -250,6 +316,7 @@ describe("prepareMcpConfig", () => {
repo: "test-repo",
branch: "test-branch",
additionalMcpConfig: arrayJson,
allowedTools: [],
});
const parsed = JSON.parse(result);
@@ -258,7 +325,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);
@@ -295,12 +362,13 @@ describe("prepareMcpConfig", () => {
repo: "test-repo",
branch: "test-branch",
additionalMcpConfig: additionalConfig,
allowedTools: [],
});
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");
@@ -315,6 +383,7 @@ describe("prepareMcpConfig", () => {
owner: "test-owner",
repo: "test-repo",
branch: "test-branch",
allowedTools: [],
});
const parsed = JSON.parse(result);
@@ -334,6 +403,7 @@ describe("prepareMcpConfig", () => {
owner: "test-owner",
repo: "test-repo",
branch: "test-branch",
allowedTools: [],
});
const parsed = JSON.parse(result);

4
test/mockContext.ts
View File

@@ -11,8 +11,8 @@ const defaultInputs = {
triggerPhrase: "/claude",
assigneeTrigger: "",
anthropicModel: "claude-3-7-sonnet-20250219",
allowedTools: "",
disallowedTools: "",
allowedTools: [] as string[],
disallowedTools: [] as string[],
customInstructions: "",
directPrompt: "",
useBedrock: false,

4
test/permissions.test.ts
View File

@@ -62,8 +62,8 @@ describe("checkWritePermissions", () => {
inputs: {
triggerPhrase: "@claude",
assigneeTrigger: "",
allowedTools: "",
disallowedTools: "",
allowedTools: [],
disallowedTools: [],
customInstructions: "",
directPrompt: "",
},

2
test/prepare-context.test.ts
View File

@@ -242,7 +242,7 @@ describe("parseEnvVarsWithContext", () => {
...mockPullRequestCommentContext,
inputs: {
...mockPullRequestCommentContext.inputs,
allowedTools: "Tool1,Tool2",
allowedTools: ["Tool1", "Tool2"],
},
});
const result = prepareContext(contextWithAllowedTools, "12345");

20
test/trigger-validation.test.ts
View File

@@ -30,8 +30,8 @@ describe("checkContainsTrigger", () => {
triggerPhrase: "/claude",
assigneeTrigger: "",
directPrompt: "Fix the bug in the login form",
allowedTools: "",
disallowedTools: "",
allowedTools: [],
disallowedTools: [],
customInstructions: "",
},
});
@@ -56,8 +56,8 @@ describe("checkContainsTrigger", () => {
triggerPhrase: "/claude",
assigneeTrigger: "",
directPrompt: "",
allowedTools: "",
disallowedTools: "",
allowedTools: [],
disallowedTools: [],
customInstructions: "",
},
});
@@ -228,8 +228,8 @@ describe("checkContainsTrigger", () => {
triggerPhrase: "@claude",
assigneeTrigger: "",
directPrompt: "",
allowedTools: "",
disallowedTools: "",
allowedTools: [],
disallowedTools: [],
customInstructions: "",
},
});
@@ -255,8 +255,8 @@ describe("checkContainsTrigger", () => {
triggerPhrase: "@claude",
assigneeTrigger: "",
directPrompt: "",
allowedTools: "",
disallowedTools: "",
allowedTools: [],
disallowedTools: [],
customInstructions: "",
},
});
@@ -282,8 +282,8 @@ describe("checkContainsTrigger", () => {
triggerPhrase: "@claude",
assigneeTrigger: "",
directPrompt: "",
allowedTools: "",
disallowedTools: "",
allowedTools: [],
disallowedTools: [],
customInstructions: "",
},
});