diff --git a/src/modes/agent/parse-tools.ts b/src/modes/agent/parse-tools.ts index 639c9131..013fda5d 100644 --- a/src/modes/agent/parse-tools.ts +++ b/src/modes/agent/parse-tools.ts @@ -1,29 +1,77 @@ -export function parseAllowedTools(claudeArgs: string): string[] { - // Match --allowedTools or --allowed-tools followed by the value - // Handle both quoted and unquoted values - // Use /g flag to find ALL occurrences, not just the first one - const patterns = [ - /--(?:allowedTools|allowed-tools)\s+"([^"]+)"/g, // Double quoted - /--(?:allowedTools|allowed-tools)\s+'([^']+)'/g, // Single quoted - /--(?:allowedTools|allowed-tools)\s+([^'"\s][^\s]*)/g, // Unquoted (must not start with quote) - ]; +import { parse as parseShellArgs } from "shell-quote"; +// Flags whose values make up the allowed-tools list. +// Include both camelCase and hyphenated variants for CLI compatibility. +const ALLOWED_TOOLS_FLAGS = new Set(["allowedTools", "allowed-tools"]); + +/** + * Strip comment lines from a shell argument string. + * Lines whose first non-whitespace character is `#` are removed entirely. + * Mirrors stripShellComments in base-action/src/parse-sdk-options.ts. + */ +function stripShellComments(input: string): string { + return input + .split("\n") + .filter((line) => !line.trim().startsWith("#")) + .join("\n"); +} + +/** + * Tokenize a claude_args string the same way base-action/src/parse-sdk-options.ts + * does: strip full comment lines, then run shell-quote. shell-quote returns + * unquoted glob patterns (e.g. `mcp__github__*`) as `{ op: "glob", pattern }` + * objects rather than strings, so recover their literal text; drop operator + * tokens (`|`, `>`, `;`, ...) which carry no value. + */ +function tokenize(claudeArgs: string): string[] { + return parseShellArgs(stripShellComments(claudeArgs)) + .map((token) => { + if (typeof token === "string") return token; + if (token && typeof token === "object" && "pattern" in token) { + return (token as { pattern: string }).pattern; + } + return null; + }) + .filter((token): token is string => token !== null); +} + +/** + * Parse the list of allowed tool names from a user-provided claude_args string. + * + * This is used to decide which GitHub MCP servers to install. It MUST stay in + * agreement with how the actual tool list is built for the SDK in + * base-action/src/parse-sdk-options.ts (parseClaudeArgsToExtraArgs): otherwise a + * tool can be granted to Claude without its MCP server being installed, or a + * server can be installed for a tool that was never granted (#1357). + * + * To stay in agreement it uses the same shell-quote tokenizer and the same + * "an accumulating flag consumes all consecutive non-flag values" semantics, + * so `--allowedTools "Read" "Grep" "mcp__github__get_commit"` captures all + * three values, and commented-out lines are ignored. + */ +export function parseAllowedTools(claudeArgs: string): string[] { + if (!claudeArgs?.trim()) return []; + + const args = tokenize(claudeArgs); const tools: string[] = []; const seen = new Set(); - for (const pattern of patterns) { - for (const match of claudeArgs.matchAll(pattern)) { - if (match[1]) { - // Don't add if the value starts with -- (another flag) - if (match[1].startsWith("--")) { - continue; - } - for (const tool of match[1].split(",")) { - const trimmed = tool.trim(); - if (trimmed && !seen.has(trimmed)) { - seen.add(trimmed); - tools.push(trimmed); - } + for (let i = 0; i < args.length; i++) { + const arg = args[i]; + if (!arg?.startsWith("--")) continue; + + const flag = arg.slice(2); + if (!ALLOWED_TOOLS_FLAGS.has(flag)) continue; + + // Consume all consecutive non-flag values, e.g. + // --allowedTools "Read" "Grep" "mcp__github__get_commit" + while (i + 1 < args.length && !args[i + 1]!.startsWith("--")) { + i++; + for (const tool of args[i]!.split(",")) { + const trimmed = tool.trim(); + if (trimmed && !seen.has(trimmed)) { + seen.add(trimmed); + tools.push(trimmed); } } } diff --git a/test/modes/parse-tools.test.ts b/test/modes/parse-tools.test.ts index 84916fb1..cc854729 100644 --- a/test/modes/parse-tools.test.ts +++ b/test/modes/parse-tools.test.ts @@ -37,9 +37,43 @@ describe("parseAllowedTools", () => { test("handles --allowedTools followed by another --allowedTools flag", () => { const args = "--allowedTools --allowedTools mcp__github__*"; - // The second --allowedTools is consumed as a value of the first, then skipped. - // This is an edge case with malformed input - returns empty. - expect(parseAllowedTools(args)).toEqual([]); + // The first --allowedTools has no value (the next token is another flag); + // the second consumes mcp__github__*. This matches how the SDK option + // parser (parse-sdk-options.ts) reads the same input. + expect(parseAllowedTools(args)).toEqual(["mcp__github__*"]); + }); + + test("captures multiple values after a single --allowedTools flag", () => { + // Regression for #1357: the install-decision parser must capture every + // value, not just the first, so it agrees with the tools actually granted + // to Claude. Previously only "Read" was seen, so the github MCP server was + // not installed even though mcp__github__get_commit was granted. + const args = '--allowedTools "Read" "Grep" "mcp__github__get_commit"'; + expect(parseAllowedTools(args)).toEqual([ + "Read", + "Grep", + "mcp__github__get_commit", + ]); + }); + + test("captures multiple values spread across lines under one flag", () => { + const args = `--allowedTools + "Read" + "Grep" + "mcp__github__get_commit"`; + expect(parseAllowedTools(args)).toEqual([ + "Read", + "Grep", + "mcp__github__get_commit", + ]); + }); + + test("ignores commented-out lines", () => { + // Regression for #1357: a commented-out flag must not be counted, matching + // the SDK parser which strips comment lines before parsing. + const args = `# --allowedTools "mcp__github__get_commit" +--allowedTools "Read"`; + expect(parseAllowedTools(args)).toEqual(["Read"]); }); test("parses multiple separate --allowed-tools flags", () => {