fix: read and merge file-path MCP configs instead of dropping them

base-action/src/parse-sdk-options.ts

@@ -1,4 +1,5 @@
 import { parse as parseShellArgs } from "shell-quote";
+import { readFileSync } from "fs";
 import type { ClaudeOptions } from "./run-claude";
 import type { Options as SdkOptions } from "@anthropic-ai/claude-agent-sdk";
 
@@ -31,12 +32,11 @@ type McpConfig = {
 /**
  * Merge multiple MCP config values into a single config.
  * Each config can be a JSON string or a file path.
- * For JSON strings, mcpServers objects are merged.
- * For file paths, they are kept as-is (user's file takes precedence and is used last).
+ * All configs are merged into a single mcpServers object.
+ * File paths are read from disk and their mcpServers are merged in.
  */
 function mergeMcpConfigs(configValues: string[]): string {
   const merged: McpConfig = { mcpServers: {} };
-  let lastFilePath: string | null = null;
 
   for (const config of configValues) {
     const trimmed = config.trim();
@@ -51,32 +51,33 @@ function mergeMcpConfigs(configValues: string[]): string {
         }
       } catch {
         // If JSON parsing fails, treat as file path
-        lastFilePath = trimmed;
+        mergeFromFile(trimmed, merged);
       }
     } else {
-      // It's a file path - store it to handle separately
-      lastFilePath = trimmed;
+      // It's a file path - read and merge its contents
+      mergeFromFile(trimmed, merged);
     }
   }
 
-  // If we have file paths, we need to keep the merged JSON and let the file
-  // be handled separately. Since we can only return one value, merge what we can.
-  // If there's a file path, we need a different approach - read the file at runtime.
-  // For now, if there's a file path, we'll stringify the merged config.
-  // The action prepends its config as JSON, so we can safely merge inline JSON configs.
+  return JSON.stringify(merged);
+}
 
-  // If no inline configs were found (all file paths), return the last file path
-  if (Object.keys(merged.mcpServers!).length === 0 && lastFilePath) {
-    return lastFilePath;
+/**
+ * Read an MCP config file and merge its mcpServers into the target config.
+ * Logs a warning and continues if the file cannot be read or parsed.
+ */
+function mergeFromFile(filePath: string, target: McpConfig): void {
+  try {
+    const content = readFileSync(filePath, "utf-8");
+    const parsed = JSON.parse(content) as McpConfig;
+    if (parsed.mcpServers) {
+      Object.assign(target.mcpServers!, parsed.mcpServers);
+    }
+  } catch (error) {
+    console.warn(
+      `Warning: Could not read or parse MCP config file "${filePath}": ${error}`,
+    );
   }
-
-  // Note: If user passes a file path, we cannot merge it at parse time since
-  // we don't have access to the file system here. The action's built-in MCP
-  // servers are always passed as inline JSON, so they will be merged.
-  // If user also passes inline JSON, it will be merged.
-  // If user passes a file path, they should ensure it includes all needed servers.
-
-  return JSON.stringify(merged);
 }
 
 /**

base-action/test/parse-sdk-options.test.ts

@@ -1,9 +1,15 @@
 #!/usr/bin/env bun
 
-import { describe, test, expect } from "bun:test";
+import { describe, test, expect, beforeAll, afterAll } from "bun:test";
+import { writeFileSync, mkdirSync, unlinkSync, rmdirSync } from "fs";
 import { parseSdkOptions } from "../src/parse-sdk-options";
 import type { ClaudeOptions } from "../src/run-claude";
 
+// Temp dir for MCP config file tests
+const MCP_TEST_DIR = "/tmp/mcp-config-test";
+const MCP_NOTION_FILE = `${MCP_TEST_DIR}/notion.json`;
+const MCP_SLACK_FILE = `${MCP_TEST_DIR}/slack.json`;
+
 describe("parseSdkOptions", () => {
   describe("allowedTools merging", () => {
     test("should extract allowedTools from claudeArgs", () => {
@@ -177,6 +183,36 @@ describe("parseSdkOptions", () => {
   });
 
   describe("mcp-config merging", () => {
+    beforeAll(() => {
+      mkdirSync(MCP_TEST_DIR, { recursive: true });
+      writeFileSync(
+        MCP_NOTION_FILE,
+        JSON.stringify({
+          mcpServers: {
+            notion: { command: "npx", args: ["notion-mcp-server"] },
+          },
+        }),
+      );
+      writeFileSync(
+        MCP_SLACK_FILE,
+        JSON.stringify({
+          mcpServers: {
+            slack: { command: "npx", args: ["slack-mcp-server"] },
+          },
+        }),
+      );
+    });
+
+    afterAll(() => {
+      try {
+        unlinkSync(MCP_NOTION_FILE);
+        unlinkSync(MCP_SLACK_FILE);
+        rmdirSync(MCP_TEST_DIR);
+      } catch {
+        // ignore cleanup errors
+      }
+    });
+
     test("should pass through single mcp-config in extraArgs", () => {
       const options: ClaudeOptions = {
         claudeArgs: `--mcp-config '{"mcpServers":{"server1":{"command":"cmd1"}}}'`,
@@ -221,33 +257,62 @@ describe("parseSdkOptions", () => {
       expect(mcpConfig.mcpServers).toHaveProperty("server3");
     });
 
-    test("should handle mcp-config file path when no inline JSON exists", () => {
+    test("should pass through single file path to CLI as-is", () => {
+      // Single mcp-config values are not merged — passed through for CLI to handle
       const options: ClaudeOptions = {
-        claudeArgs: `--mcp-config /tmp/user-mcp-config.json`,
+        claudeArgs: `--mcp-config ${MCP_NOTION_FILE}`,
       };
 
       const result = parseSdkOptions(options);
 
-      expect(result.sdkOptions.extraArgs?.["mcp-config"]).toBe(
-        "/tmp/user-mcp-config.json",
-      );
+      expect(result.sdkOptions.extraArgs?.["mcp-config"]).toBe(MCP_NOTION_FILE);
     });
 
-    test("should merge inline JSON configs when file path is also present", () => {
-      // When action provides inline JSON and user provides a file path,
-      // the inline JSON configs should be merged (file paths cannot be merged at parse time)
+    test("should merge inline JSON configs and file path configs together", () => {
+      // This is the exact scenario from issue #1191: action provides inline JSON
+      // for github_comment, user provides a file path for their MCP server
       const options: ClaudeOptions = {
-        claudeArgs: `--mcp-config '{"mcpServers":{"github_comment":{"command":"node"}}}' --mcp-config '{"mcpServers":{"github_ci":{"command":"node"}}}' --mcp-config /tmp/user-config.json`,
+        claudeArgs: `--mcp-config '{"mcpServers":{"github_comment":{"command":"node"}}}' --mcp-config '{"mcpServers":{"github_ci":{"command":"node"}}}' --mcp-config ${MCP_NOTION_FILE}`,
       };
 
       const result = parseSdkOptions(options);
 
-      // The inline JSON configs should be merged
       const mcpConfig = JSON.parse(
         result.sdkOptions.extraArgs?.["mcp-config"] as string,
       );
+      // All servers should be present — including the file-path one
       expect(mcpConfig.mcpServers).toHaveProperty("github_comment");
       expect(mcpConfig.mcpServers).toHaveProperty("github_ci");
+      expect(mcpConfig.mcpServers).toHaveProperty("notion");
+      expect(mcpConfig.mcpServers.notion.command).toBe("npx");
+    });
+
+    test("should merge multiple file paths", () => {
+      const options: ClaudeOptions = {
+        claudeArgs: `--mcp-config ${MCP_NOTION_FILE} --mcp-config ${MCP_SLACK_FILE}`,
+      };
+
+      const result = parseSdkOptions(options);
+
+      const mcpConfig = JSON.parse(
+        result.sdkOptions.extraArgs?.["mcp-config"] as string,
+      );
+      expect(mcpConfig.mcpServers).toHaveProperty("notion");
+      expect(mcpConfig.mcpServers).toHaveProperty("slack");
+    });
+
+    test("should warn and continue when file path does not exist", () => {
+      const options: ClaudeOptions = {
+        claudeArgs: `--mcp-config '{"mcpServers":{"github_comment":{"command":"node"}}}' --mcp-config /tmp/nonexistent-mcp-config.json`,
+      };
+
+      const result = parseSdkOptions(options);
+
+      // Should still have the inline config, file path failure is non-fatal
+      const mcpConfig = JSON.parse(
+        result.sdkOptions.extraArgs?.["mcp-config"] as string,
+      );
+      expect(mcpConfig.mcpServers).toHaveProperty("github_comment");
     });
 
     test("should handle mcp-config with other flags", () => {