diff --git a/base-action/src/parse-sdk-options.ts b/base-action/src/parse-sdk-options.ts index ec65b8fbb..cbccdc5cb 100644 --- a/base-action/src/parse-sdk-options.ts +++ b/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); } /** diff --git a/base-action/test/parse-sdk-options.test.ts b/base-action/test/parse-sdk-options.test.ts index c74d98e9c..16827869c 100644 --- a/base-action/test/parse-sdk-options.test.ts +++ b/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", () => {