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
2026-01-22 22:44:13 +08:00 · 2025-06-04 07:52:07 -07:00
parent 2c0d6b1a1e
commit c685888c3d
10 changed files with 89 additions and 90 deletions
--- a/src/create-prompt/index.ts
+++ b/src/create-prompt/index.ts
@@ -35,38 +35,35 @@ const BASE_ALLOWED_TOOLS = [
 ];
 const DISALLOWED_TOOLS = ["WebSearch", "WebFetch"];

-export function buildAllowedToolsString(customAllowedTools?: string): string {
+export function buildAllowedToolsString(customAllowedTools?: string[]): string {
  let baseTools = [...BASE_ALLOWED_TOOLS];

  let allAllowedTools = baseTools.join(",");
-  if (customAllowedTools) {
-    allAllowedTools = `${allAllowedTools},${customAllowedTools}`;
+  if (customAllowedTools && customAllowedTools.length > 0) {
+    allAllowedTools = `${allAllowedTools},${customAllowedTools.join(",")}`;
  }
  return allAllowedTools;
 }

 export function buildDisallowedToolsString(
-  customDisallowedTools?: string,
-  allowedTools?: string,
+  customDisallowedTools?: string[],
+  allowedTools?: string[],
 ): string {
  let disallowedTools = [...DISALLOWED_TOOLS];

  // If user has explicitly allowed some hardcoded disallowed tools, remove them from disallowed list
-  if (allowedTools) {
-    const allowedToolsArray = allowedTools
-      .split(",")
-      .map((tool) => tool.trim());
+  if (allowedTools && allowedTools.length > 0) {
    disallowedTools = disallowedTools.filter(
-      (tool) => !allowedToolsArray.includes(tool),
+      (tool) => !allowedTools.includes(tool),
    );
  }

  let allDisallowedTools = disallowedTools.join(",");
-  if (customDisallowedTools) {
+  if (customDisallowedTools && customDisallowedTools.length > 0) {
    if (allDisallowedTools) {
-      allDisallowedTools = `${allDisallowedTools},${customDisallowedTools}`;
+      allDisallowedTools = `${allDisallowedTools},${customDisallowedTools.join(",")}`;
    } else {
-      allDisallowedTools = customDisallowedTools;
+      allDisallowedTools = customDisallowedTools.join(",");
    }
  }
  return allDisallowedTools;
@@ -120,8 +117,8 @@ export function prepareContext(
    triggerPhrase,
    ...(triggerUsername && { triggerUsername }),
    ...(customInstructions && { customInstructions }),
-    ...(allowedTools && { allowedTools }),
-    ...(disallowedTools && { disallowedTools }),
+    ...(allowedTools.length > 0 && { allowedTools: allowedTools.join(",") }),
+    ...(disallowedTools.length > 0 && { disallowedTools: disallowedTools.join(",") }),
    ...(directPrompt && { directPrompt }),
    ...(claudeBranch && { claudeBranch }),
  };
@@ -636,11 +633,11 @@ export async function createPrompt(

    // Set allowed tools
    const allAllowedTools = buildAllowedToolsString(
-      preparedContext.allowedTools,
+      context.inputs.allowedTools,
    );
    const allDisallowedTools = buildDisallowedToolsString(
-      preparedContext.disallowedTools,
-      preparedContext.allowedTools,
+      context.inputs.disallowedTools,
+      context.inputs.allowedTools,
    );

    core.exportVariable("ALLOWED_TOOLS", allAllowedTools);
--- a/src/entrypoints/prepare.ts
+++ b/src/entrypoints/prepare.ts
@@ -85,7 +85,6 @@ 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,
@@ -93,7 +92,7 @@ async function run() {
      branch: branchInfo.currentBranch,
      additionalMcpConfig,
      claudeCommentId: commentId.toString(),
-      allowedTools,
+      allowedTools: context.inputs.allowedTools,
    });
    core.setOutput("mcp_config", mcpConfig);
  } catch (error) {
--- a/src/github/context.ts
+++ b/src/github/context.ts
@@ -28,8 +28,8 @@ export type ParsedGitHubContext = {
  inputs: {
    triggerPhrase: string;
    assigneeTrigger: string;
-    allowedTools: string;
-    disallowedTools: string;
+    allowedTools: string[];
+    disallowedTools: string[];
    customInstructions: string;
    directPrompt: string;
    baseBranch?: string;
@@ -52,8 +52,14 @@ export function parseGitHubContext(): ParsedGitHubContext {
    inputs: {
      triggerPhrase: process.env.TRIGGER_PHRASE ?? "@claude",
      assigneeTrigger: process.env.ASSIGNEE_TRIGGER ?? "",
-      allowedTools: process.env.ALLOWED_TOOLS ?? "",
-      disallowedTools: process.env.DISALLOWED_TOOLS ?? "",
+      allowedTools: (process.env.ALLOWED_TOOLS ?? "")
+        .split(",")
+        .map((tool) => tool.trim())
+        .filter((tool) => tool.length > 0),
+      disallowedTools: (process.env.DISALLOWED_TOOLS ?? "")
+        .split(",")
+        .map((tool) => tool.trim())
+        .filter((tool) => tool.length > 0),
      customInstructions: process.env.CUSTOM_INSTRUCTIONS ?? "",
      directPrompt: process.env.DIRECT_PROMPT ?? "",
      baseBranch: process.env.BASE_BRANCH,
--- a/src/mcp/install-mcp-server.ts
+++ b/src/mcp/install-mcp-server.ts
@@ -7,7 +7,7 @@ type PrepareConfigParams = {
  branch: string;
  additionalMcpConfig?: string;
  claudeCommentId?: string;
-  allowedTools?: string;
+  allowedTools: string[];
 };

 export async function prepareMcpConfig(
@@ -23,22 +23,36 @@ export async function prepareMcpConfig(
    allowedTools,
  } = params;
  try {
-    // Parse allowed tools into an array
-    const allowedToolsList = allowedTools
-      ? allowedTools.split(",").map((tool) => tool.trim())
-      : [];
+    const allowedToolsList = allowedTools || [];

-    // Check if any GitHub MCP tools are allowed
    const hasGitHubMcpTools = allowedToolsList.some((tool) =>
      tool.startsWith("mcp__github__"),
    );

-    // Start with an empty servers object
-    const mcpServers: Record<string, any> = {};
+    const baseMcpConfig: { mcpServers: Record<string, unknown> } = {
+      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",
+          },
+        },
+      },
+    };

-    // Only include github MCP server if GitHub tools are allowed
    if (hasGitHubMcpTools) {
-      mcpServers.github = {
+      baseMcpConfig.mcpServers.github = {
        command: "docker",
        args: [
          "run",
@@ -54,31 +68,6 @@ export async function prepareMcpConfig(
      };
    }

-    // 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 and should always be available
-    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,
-    };
-
    // Merge with additional MCP config if provided
    if (additionalMcpConfig && additionalMcpConfig.trim()) {
      try {
--- a/test/create-prompt.test.ts
+++ b/test/create-prompt.test.ts
@@ -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,
--- a/test/install-mcp-server.test.ts
+++ b/test/install-mcp-server.test.ts
@@ -52,8 +52,10 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
-      allowedTools:
-        "mcp__github__create_issue,mcp__github_file_ops__commit_files",
+      allowedTools: [
+        "mcp__github__create_issue",
+        "mcp__github_file_ops__commit_files",
+      ],
    });

    const parsed = JSON.parse(result);
@@ -71,8 +73,10 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
-      allowedTools:
-        "mcp__github_file_ops__commit_files,mcp__github_file_ops__update_claude_comment",
+      allowedTools: [
+        "mcp__github_file_ops__commit_files",
+        "mcp__github_file_ops__update_claude_comment",
+      ],
    });

    const parsed = JSON.parse(result);
@@ -87,7 +91,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
-      allowedTools: "Edit,Read,Write",
+      allowedTools: ["Edit", "Read", "Write"],
    });

    const parsed = JSON.parse(result);
@@ -147,8 +151,10 @@ describe("prepareMcpConfig", () => {
      repo: "test-repo",
      branch: "test-branch",
      additionalMcpConfig: additionalConfig,
-      allowedTools:
-        "mcp__github__create_issue,mcp__github_file_ops__commit_files",
+      allowedTools: [
+        "mcp__github__create_issue",
+        "mcp__github_file_ops__commit_files",
+      ],
    });

    const parsed = JSON.parse(result);
@@ -182,8 +188,10 @@ describe("prepareMcpConfig", () => {
      repo: "test-repo",
      branch: "test-branch",
      additionalMcpConfig: additionalConfig,
-      allowedTools:
-        "mcp__github__create_issue,mcp__github_file_ops__commit_files",
+      allowedTools: [
+        "mcp__github__create_issue",
+        "mcp__github_file_ops__commit_files",
+      ],
    });

    const parsed = JSON.parse(result);
--- a/test/mockContext.ts
+++ b/test/mockContext.ts
@@ -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,
--- a/test/permissions.test.ts
+++ b/test/permissions.test.ts
@@ -62,8 +62,8 @@ describe("checkWritePermissions", () => {
    inputs: {
      triggerPhrase: "@claude",
      assigneeTrigger: "",
-      allowedTools: "",
-      disallowedTools: "",
+      allowedTools: [],
+      disallowedTools: [],
      customInstructions: "",
      directPrompt: "",
    },
--- a/test/prepare-context.test.ts
+++ b/test/prepare-context.test.ts
@@ -242,7 +242,7 @@ describe("parseEnvVarsWithContext", () => {
        ...mockPullRequestCommentContext,
        inputs: {
          ...mockPullRequestCommentContext.inputs,
-          allowedTools: "Tool1,Tool2",
+          allowedTools: ["Tool1", "Tool2"],
        },
      });
      const result = prepareContext(contextWithAllowedTools, "12345");
--- a/test/trigger-validation.test.ts
+++ b/test/trigger-validation.test.ts
@@ -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: "",
        },
      });