Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
63 changes: 63 additions & 0 deletions actions/setup/js/mcp_cli_bridge.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,61 @@ function resolveSchemaPropertyKey(key, schemaProperties, normalizedSchemaKeyMap,
return normalizedSchemaKeyMap.get(normalized) || key;
}

const CLI_UNESCAPED_TEXT_ARG_KEYS = new Set(["body", "draftbody"]);

/**
* Unescape a conservative subset of JSON-style escape sequences in a CLI text argument.
*
* This is only applied to body-like text fields where authors commonly expect
* `\n` and similar escapes to become literal formatting characters, matching
* JSON stdin mode more closely without mutating unrelated string arguments
* such as file paths or regex patterns.
*
* Supported sequences:
* `\n` → newline
* `\t` → tab
* `\r` → carriage return
* `\b` → backspace
* `\f` → form feed
* `\\` → single backslash
* `\uXXXX` → Unicode code point
* Any other `\X` is left unchanged (the backslash is preserved).
*
* @param {string} str - Raw CLI string argument
* @returns {string} String with supported escape sequences replaced by literal characters
*/
function unescapeCliStringArg(str) {
return str.replace(/\\(?:u([0-9a-fA-F]{4})|([\s\S]))/g, (match, hex, char) => {
if (hex) {
return String.fromCharCode(Number.parseInt(hex, 16));
}
switch (char) {
case "n":
return "\n";
case "t":
return "\t";
case "r":
return "\r";
case "b":
return "\b";
case "f":
return "\f";
case "\\":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The PR claims parity with JSON.parse, but \b (backspace), \f (form feed), and \uXXXX (Unicode) are valid JSON escape sequences that JSON.parse handles yet unescapeCliStringArg leaves unchanged.

💡 Example gap
// JSON stdin mode
JSON.parse('"\u2019"') // → right single quote \u2019

// CLI mode after this PR
unescapeCliStringArg("\u2019") // → "\u2019" (unchanged)

If agents ever pass Unicode escapes or \b/\f via CLI flags, those will silently differ from JSON stdin mode. Consider adding \b, \f, and a \uXXXX handler, or documenting the known gap in the JSDoc.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in da0a061: unescapeCliStringArg now also handles \b, \f, and \uXXXX, and the tests cover those escape sequences.

return "\\";
default:
return match;
}
});
}

/**
* @param {string} key
* @returns {boolean}
*/
function shouldUnescapeCliTextArg(key) {
return CLI_UNESCAPED_TEXT_ARG_KEYS.has(normalizeSchemaKey(key));
}

/**
* Parse and coerce a CLI argument value based on the MCP tool schema property type.
*
Expand Down Expand Up @@ -857,6 +912,13 @@ function coerceToolArgValue(key, rawValue, schemaProperty, existingValue, allowN
}
}

// Only unescape body-like text fields. Shell argv is already decoded, so
// applying a second escape pass to arbitrary string fields would corrupt
// values like Windows paths and regex patterns.
if (types.includes("string") && shouldUnescapeCliTextArg(key)) {
return unescapeCliStringArg(rawValue);
}

// When schema metadata is unavailable (e.g. empty tools cache), apply
// conservative numeric coercion fallback for CLI ergonomics.
if (allowNumericFallback && types.length === 0) {
Expand Down Expand Up @@ -1376,6 +1438,7 @@ if (require.main === module) {
module.exports = {
parseToolArgs,
coerceToolArgValue,
unescapeCliStringArg,
extractJSONRPCMessages,
renderProgressMessages,
formatResponse,
Expand Down
106 changes: 105 additions & 1 deletion actions/setup/js/mcp_cli_bridge.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,19 @@ import fs from "fs";
import os from "os";
import path from "path";

import { ensureSafeOutputsTools, formatResponse, getToolCallTimeoutMs, hasStdinJsonPayload, parseToolArgs, readStdinSync, shouldShowToolHelpForEmptyArgs, showHelp, showToolHelp, writeStdoutAndFlush } from "./mcp_cli_bridge.cjs";
import {
ensureSafeOutputsTools,
formatResponse,
getToolCallTimeoutMs,
hasStdinJsonPayload,
parseToolArgs,
readStdinSync,
shouldShowToolHelpForEmptyArgs,
showHelp,
showToolHelp,
unescapeCliStringArg,
writeStdoutAndFlush,
} from "./mcp_cli_bridge.cjs";

describe("mcp_cli_bridge.cjs", () => {
let originalCore;
Expand Down Expand Up @@ -750,6 +762,98 @@ describe("mcp_cli_bridge.cjs", () => {
});
});

describe("unescapeCliStringArg", () => {
it("converts \\n to an actual newline", () => {
expect(unescapeCliStringArg("Hello\\nWorld")).toBe("Hello\nWorld");
});

it("converts \\t to a tab character", () => {
expect(unescapeCliStringArg("col1\\tcol2")).toBe("col1\tcol2");
});

it("converts \\r to a carriage return", () => {
expect(unescapeCliStringArg("line1\\rline2")).toBe("line1\rline2");
});

it("converts \\b to a backspace character", () => {
expect(unescapeCliStringArg("abc\\bdef")).toBe("abc\bdef");
});

it("converts \\f to a form-feed character", () => {
expect(unescapeCliStringArg("page1\\fpage2")).toBe("page1\fpage2");
});

it("converts \\\\ to a single backslash", () => {
expect(unescapeCliStringArg("path\\\\to\\\\file")).toBe("path\\to\\file");
});

it("converts \\uXXXX escapes to their Unicode code points", () => {
expect(unescapeCliStringArg("quote:\\u2019")).toBe("quote:’");
});

it("converts \\\\n to a literal backslash followed by n (not a newline)", () => {
// \\n in the CLI arg should become \n (backslash + n), not a newline
expect(unescapeCliStringArg("Hello\\\\nWorld")).toBe("Hello\\nWorld");
});

it("leaves unknown escape sequences unchanged", () => {
expect(unescapeCliStringArg("value\\xunknown")).toBe("value\\xunknown");
});

it("handles multiple escape sequences in the same string", () => {
expect(unescapeCliStringArg("line1\\nline2\\nline3")).toBe("line1\nline2\nline3");
});

it("returns a plain string unchanged when no escape sequences are present", () => {
expect(unescapeCliStringArg("no escapes here")).toBe("no escapes here");
});
});

describe("parseToolArgs — body-like string escape unescaping", () => {
it("unescapes \\n in body CLI flag arguments", () => {
const schemaProperties = { body: { type: "string" } };
const { args } = parseToolArgs(["--body", "Hello\\nWorld"], schemaProperties);
expect(args).toEqual({ body: "Hello\nWorld" });
});

it("unescapes \\n in body --key=value arguments", () => {
const schemaProperties = { body: { type: "string" } };
const { args } = parseToolArgs(["--body=Hello\\nWorld"], schemaProperties);
expect(args).toEqual({ body: "Hello\nWorld" });
});

it("unescapes \\n for nullable draft body fields", () => {
const schemaProperties = { draft_body: { type: ["string", "null"] } };
const { args } = parseToolArgs(["--draft-body", "line1\\nline2"], schemaProperties);
expect(args).toEqual({ draft_body: "line1\nline2" });
});

it("does not unescape generic string fields like paths", () => {
const schemaProperties = { path: { type: "string" } };
const { args } = parseToolArgs(["--path", "C:\\temp\\new_file"], schemaProperties);
expect(args).toEqual({ path: "C:\\temp\\new_file" });
});

it("does not unescape \\n when schema type is integer", () => {
// A value with \n for an integer field should not be unescaped (it would fail coercion anyway)
const schemaProperties = { count: { type: "integer" } };
const { args } = parseToolArgs(["--count", "5\\n"], schemaProperties);
// "5\n" is not a valid integer, falls through to rawValue
expect(args).toEqual({ count: "5\\n" });
});

it("produces actual newlines in body fields matching JSON stdin mode behaviour", () => {
// Verify that --body "title\n\nbody" (CLI flags) gives the same result as
// JSON stdin with {"body":"title\n\nbody"}
const schemaProperties = { body: { type: ["string", "null"] } };

const { args: cliArgs } = parseToolArgs(["--body", "title\\n\\nbody"], schemaProperties);
const { args: jsonArgs } = parseToolArgs(["."], schemaProperties, '{"body":"title\\n\\nbody"}');

expect(cliArgs).toEqual(jsonArgs);
});
});

describe("writeStdoutAndFlush", () => {
it("resolves immediately when stdout.write returns true (no backpressure)", async () => {
// The beforeEach mock captures chunks and returns true (no backpressure).
Expand Down
Loading