Skip to content

fix(mcp): spawn bundled MCP servers on pull_request events #1261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MukundaKatta wants to merge 1 commit into
base: main
Choose a base branch
from
+132 −78
Open

fix(mcp): spawn bundled MCP servers on pull_request events #1261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 73 additions & 35 deletions base-action/src/parse-sdk-options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,39 @@ 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).
* Result of splitting --mcp-config values into the SDK-native inline path
* and the CLI-handled file-path passthrough.
*
* inlineServers: parsed mcpServers entries from inline JSON values (merged).
* These are routed to Options.mcpServers — the SDK's typed, in-process MCP
* registration path. Going through the typed API avoids the brittle CLI
* forwarding of --mcp-config '{...}' from extraArgs, which is the failure
* mode reported in #1245 / #1191 (action-prepended JSON correctly stored
* in extraArgs but the spawned CLI's session init still reports
* `mcp_servers: []`).
*
* filePath: at most one --mcp-config file path. File paths cannot be merged
* at parse time (no fs access here) and are forwarded to the CLI via
* extraArgs["mcp-config"] unchanged. The CLI loads and merges the file
* alongside any servers we registered via Options.mcpServers.
*/
function mergeMcpConfigs(configValues: string[]): string {
const merged: McpConfig = { mcpServers: {} };
let lastFilePath: string | null = null;
type SplitMcpConfigs = {
inlineServers: Record<string, unknown>;
filePath: string | null;
};

/**
* Split --mcp-config values into inline mcpServers (for Options.mcpServers)
* and at most one file path (for extraArgs passthrough to the CLI).
*
* Inline JSON across multiple --mcp-config flags is merged into a single
* mcpServers map; later entries override earlier ones on key conflict, which
* matches the previous mergeMcpConfigs() behavior so user inline JSON can
* still override action-prepended servers.
*/
function splitMcpConfigs(configValues: string[]): SplitMcpConfigs {
const inlineServers: Record<string, unknown> = {};
let filePath: string | null = null;

for (const config of configValues) {
const trimmed = config.trim();
Expand All @@ -47,36 +72,19 @@ function mergeMcpConfigs(configValues: string[]): string {
try {
const parsed = JSON.parse(trimmed) as McpConfig;
if (parsed.mcpServers) {
Object.assign(merged.mcpServers!, parsed.mcpServers);
Object.assign(inlineServers, parsed.mcpServers);
}
} catch {
// If JSON parsing fails, treat as file path
lastFilePath = trimmed;
filePath = trimmed;
}
} else {
// It's a file path - store it to handle separately
lastFilePath = trimmed;
filePath = trimmed;
}
}

// 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.

// If no inline configs were found (all file paths), return the last file path
if (Object.keys(merged.mcpServers!).length === 0 && lastFilePath) {
return lastFilePath;
}

// 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);
return { inlineServers, filePath };
}

/**
Expand Down Expand Up @@ -211,13 +219,31 @@ export function parseSdkOptions(options: ClaudeOptions): ParsedSdkOptions {
delete extraArgs["disallowedTools"];
delete extraArgs["disallowed-tools"];

// Merge multiple --mcp-config values by combining their mcpServers objects
// The action prepends its config (github_comment, github_ci, etc.) as inline JSON,
// and users may provide their own config as inline JSON or file path
// Route inline --mcp-config JSON to Options.mcpServers (SDK-native typed
// path), and leave file paths in extraArgs for the CLI to load.
//
// Background (#1245, #1191): the action prepends its bundled servers
// (github_comment, github_ci, github_inline_comment, github_file_ops,
// optional github) as a single `--mcp-config '{...}'` flag inside
// claudeArgs. Earlier this stayed in extraArgs and was meant to be
// forwarded to the spawned Claude CLI. Multiple users have reported that
// the spawned CLI's session init reports `mcp_servers: []` even though
// extraArgs["mcp-config"] is correctly populated — the CLI subprocess
// never registers the inline servers. The SDK exposes a typed,
// in-process registration path via Options.mcpServers (used by Anthropic's
// own SDK demos) which doesn't depend on stringified-JSON CLI forwarding.
// Routing inline JSON through that path makes the bundled servers spawn
// reliably; file paths still go through the CLI flag because we can't
// load them from disk here without changing this function's signature.
let inlineMcpServers: Record<string, unknown> = {};
if (extraArgs["mcp-config"]) {
const mcpConfigValues = extraArgs["mcp-config"].split(ACCUMULATE_DELIMITER);
if (mcpConfigValues.length > 1) {
extraArgs["mcp-config"] = mergeMcpConfigs(mcpConfigValues);
const { inlineServers, filePath } = splitMcpConfigs(mcpConfigValues);
inlineMcpServers = inlineServers;
if (filePath) {
extraArgs["mcp-config"] = filePath;
} else {
delete extraArgs["mcp-config"];
}
}

Expand Down Expand Up @@ -265,9 +291,21 @@ export function parseSdkOptions(options: ClaudeOptions): ParsedSdkOptions {
systemPrompt,
fallbackModel: options.fallbackModel,
pathToClaudeCodeExecutable: options.pathToClaudeCodeExecutable,
// Inline --mcp-config JSON (action-bundled + user inline) registered via
// the SDK's typed mcpServers path so the spawned CLI sees them in
// session init (`mcp_servers`). Cast keeps the Record<string, unknown>
// shape we built compatible with the SDK's narrower McpServerConfig union
// (each entry is the same {command, args, env}/{type, url, ...} shape the
// SDK already accepts via .mcp.json).
mcpServers:
Object.keys(inlineMcpServers).length > 0
? (inlineMcpServers as SdkOptions["mcpServers"])
: undefined,

// Pass through claudeArgs as extraArgs - CLI handles --mcp-config, --json-schema, etc.
// Note: allowedTools and disallowedTools have been removed from extraArgs to prevent duplicates
// Pass through claudeArgs as extraArgs - CLI handles --mcp-config (file
// paths only; inline JSON is lifted to mcpServers above), --json-schema,
// etc. Note: allowedTools and disallowedTools have been removed from
// extraArgs to prevent duplicates.
extraArgs,
env,

Expand Down
102 changes: 59 additions & 43 deletions base-action/test/parse-sdk-options.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,52 +176,57 @@ describe("parseSdkOptions", () => {
});
});

describe("mcp-config merging", () => {
test("should pass through single mcp-config in extraArgs", () => {
describe("mcp-config routing", () => {
// Inline JSON --mcp-config values are routed to sdkOptions.mcpServers
// (the SDK's typed registration path) so the spawned CLI sees them in
// session init. File paths stay in extraArgs for the CLI to load.
// Background: #1245 / #1191 — when inline JSON was forwarded via
// extraArgs["mcp-config"], the spawned CLI's session init reported
// `mcp_servers: []` despite the flag being correctly populated.
test("routes single inline mcp-config to sdkOptions.mcpServers", () => {
const options: ClaudeOptions = {
claudeArgs: `--mcp-config '{"mcpServers":{"server1":{"command":"cmd1"}}}'`,
};

const result = parseSdkOptions(options);

expect(result.sdkOptions.extraArgs?.["mcp-config"]).toBe(
'{"mcpServers":{"server1":{"command":"cmd1"}}}',
);
expect(result.sdkOptions.mcpServers).toEqual({
server1: { command: "cmd1" },
});
expect(result.sdkOptions.extraArgs?.["mcp-config"]).toBeUndefined();
});

test("should merge multiple mcp-config flags with inline JSON", () => {
test("merges multiple inline mcp-config flags into sdkOptions.mcpServers", () => {
// Simulates action prepending its config, then user providing their own
const options: ClaudeOptions = {
claudeArgs: `--mcp-config '{"mcpServers":{"github_comment":{"command":"node","args":["server.js"]}}}' --mcp-config '{"mcpServers":{"user_server":{"command":"custom","args":["run"]}}}'`,
};

const result = parseSdkOptions(options);

const mcpConfig = JSON.parse(
result.sdkOptions.extraArgs?.["mcp-config"] as string,
);
expect(mcpConfig.mcpServers).toHaveProperty("github_comment");
expect(mcpConfig.mcpServers).toHaveProperty("user_server");
expect(mcpConfig.mcpServers.github_comment.command).toBe("node");
expect(mcpConfig.mcpServers.user_server.command).toBe("custom");
const servers = result.sdkOptions.mcpServers as Record<string, any>;
expect(servers).toHaveProperty("github_comment");
expect(servers).toHaveProperty("user_server");
expect(servers.github_comment.command).toBe("node");
expect(servers.user_server.command).toBe("custom");
expect(result.sdkOptions.extraArgs?.["mcp-config"]).toBeUndefined();
});

test("should merge three mcp-config flags", () => {
test("merges three inline mcp-config flags into sdkOptions.mcpServers", () => {
const options: ClaudeOptions = {
claudeArgs: `--mcp-config '{"mcpServers":{"server1":{"command":"cmd1"}}}' --mcp-config '{"mcpServers":{"server2":{"command":"cmd2"}}}' --mcp-config '{"mcpServers":{"server3":{"command":"cmd3"}}}'`,
};

const result = parseSdkOptions(options);

const mcpConfig = JSON.parse(
result.sdkOptions.extraArgs?.["mcp-config"] as string,
);
expect(mcpConfig.mcpServers).toHaveProperty("server1");
expect(mcpConfig.mcpServers).toHaveProperty("server2");
expect(mcpConfig.mcpServers).toHaveProperty("server3");
const servers = result.sdkOptions.mcpServers as Record<string, any>;
expect(servers).toHaveProperty("server1");
expect(servers).toHaveProperty("server2");
expect(servers).toHaveProperty("server3");
expect(result.sdkOptions.extraArgs?.["mcp-config"]).toBeUndefined();
});

test("should handle mcp-config file path when no inline JSON exists", () => {
test("forwards mcp-config file path through extraArgs (no inline JSON)", () => {
const options: ClaudeOptions = {
claudeArgs: `--mcp-config /tmp/user-mcp-config.json`,
};
Expand All @@ -231,42 +236,45 @@ describe("parseSdkOptions", () => {
expect(result.sdkOptions.extraArgs?.["mcp-config"]).toBe(
"/tmp/user-mcp-config.json",
);
expect(result.sdkOptions.mcpServers).toBeUndefined();
});

test("should merge inline JSON configs when file path is also present", () => {
test("splits inline JSON to mcpServers and keeps file path in extraArgs", () => {
// 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)
// inline servers go to mcpServers and the file path stays in extraArgs
// so the CLI loads it (and merges it with what the SDK already registered)
const options: ClaudeOptions = {
claudeArgs: `--mcp-config '{"mcpServers":{"github_comment":{"command":"node"}}}' --mcp-config '{"mcpServers":{"github_ci":{"command":"node"}}}' --mcp-config /tmp/user-config.json`,
};

const result = parseSdkOptions(options);

// The inline JSON configs should be merged
const mcpConfig = JSON.parse(
result.sdkOptions.extraArgs?.["mcp-config"] as string,
const servers = result.sdkOptions.mcpServers as Record<string, any>;
expect(servers).toHaveProperty("github_comment");
expect(servers).toHaveProperty("github_ci");
expect(result.sdkOptions.extraArgs?.["mcp-config"]).toBe(
"/tmp/user-config.json",
);
expect(mcpConfig.mcpServers).toHaveProperty("github_comment");
expect(mcpConfig.mcpServers).toHaveProperty("github_ci");
});

test("should handle mcp-config with other flags", () => {
test("routes inline mcp-config to mcpServers alongside other flags", () => {
const options: ClaudeOptions = {
claudeArgs: `--mcp-config '{"mcpServers":{"server1":{}}}' --model claude-3-5-sonnet --mcp-config '{"mcpServers":{"server2":{}}}'`,
};

const result = parseSdkOptions(options);

const mcpConfig = JSON.parse(
result.sdkOptions.extraArgs?.["mcp-config"] as string,
);
expect(mcpConfig.mcpServers).toHaveProperty("server1");
expect(mcpConfig.mcpServers).toHaveProperty("server2");
const servers = result.sdkOptions.mcpServers as Record<string, any>;
expect(servers).toHaveProperty("server1");
expect(servers).toHaveProperty("server2");
expect(result.sdkOptions.extraArgs?.["mcp-config"]).toBeUndefined();
expect(result.sdkOptions.extraArgs?.["model"]).toBe("claude-3-5-sonnet");
});

test("should handle real-world scenario: action config + user config", () => {
// This is the exact scenario from the bug report
test("real-world scenario: action bundled config + user inline config", () => {
// This is the exact scenario from issue #1245 — both action-prepended
// and user-provided inline JSON should register through mcpServers so
// the spawned CLI lists them in session init.
const actionConfig = JSON.stringify({
mcpServers: {
github_comment: {
Expand All @@ -288,13 +296,21 @@ describe("parseSdkOptions", () => {

const result = parseSdkOptions(options);

const mcpConfig = JSON.parse(
result.sdkOptions.extraArgs?.["mcp-config"] as string,
);
// All servers should be present
expect(mcpConfig.mcpServers).toHaveProperty("github_comment");
expect(mcpConfig.mcpServers).toHaveProperty("github_ci");
expect(mcpConfig.mcpServers).toHaveProperty("my_custom_server");
const servers = result.sdkOptions.mcpServers as Record<string, any>;
expect(servers).toHaveProperty("github_comment");
expect(servers).toHaveProperty("github_ci");
expect(servers).toHaveProperty("my_custom_server");
expect(result.sdkOptions.extraArgs?.["mcp-config"]).toBeUndefined();
});

test("leaves mcpServers undefined when no --mcp-config is passed", () => {
const options: ClaudeOptions = {
claudeArgs: `--model claude-3-5-sonnet`,
};

const result = parseSdkOptions(options);

expect(result.sdkOptions.mcpServers).toBeUndefined();
});
});

Expand Down