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
77 changes: 71 additions & 6 deletions actions/setup/js/mcp_cli_bridge.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,63 @@ function readStdinSync() {
return Buffer.concat(chunks).toString("utf8");
}

/**
* Try to extract a specific field value from a JSON object string.
*
* When per-field stdin mode (`--key .`) is used and stdin contains a JSON
* object, this function checks whether that object has the target key. If so,
* it returns the field value, allowing agents to pipe a full JSON payload and
* extract individual fields without the entire JSON string ending up as the
* field value.
*
* Example: an agent may construct a full payload JSON and still pass individual
* fields via `--key .`:
* printf '{"title":"Fix bug","body":"Details here"}' \
* | safeoutputs create_pull_request --title "Fix bug" --body .

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.

[/codebase-design] The startsWith('{') fast-path could reject valid JSON with leading whitespace.

If trimmedStdin is already trimmed (callers use stdinContent.trim()), this is fine. But the function's JSDoc says it accepts "pre-trimmed stdin" — consider asserting or documenting this contract more explicitly so future callers don't pass un-trimmed input and hit a silent miss.

@copilot please address this.

* Without this helper the body would be the raw JSON string. With it, the
* body is correctly extracted as "Details here".
*
* Returns `undefined` when stdin is not a JSON object, when the key is absent
* from the object, or when JSON parsing fails — in all those cases the caller
* falls back to using the raw stdin content.
*
* @param {string} trimmedStdin - Pre-trimmed stdin content. Must have no
* leading whitespace — the function uses a `startsWith('{')` fast-path to
* avoid parsing non-JSON input, so leading whitespace will cause a JSON
* object to be treated as non-JSON and return `undefined`. All callers
* must pass `stdinContent.trim()` before invoking this function.
* @param {string} canonicalKey - Canonical schema key to look up
* @param {Record<string, {type?: string|string[]}>} schemaProperties - Tool input schema properties
* @param {Map<string, string>} normalizedSchemaKeyMap - Map from normalized key to canonical schema key
* @param {Set<string>} ambiguousNormalizedSchemaKeys - Normalized keys that map to multiple schema keys
* @returns {unknown} The extracted field value, or `undefined` if not found
*/
function tryExtractJsonFieldFromStdin(trimmedStdin, canonicalKey, schemaProperties, normalizedSchemaKeyMap, ambiguousNormalizedSchemaKeys) {
if (!trimmedStdin.startsWith("{")) {
return undefined;
}
try {
const parsed = JSON.parse(trimmedStdin);
if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) {
return undefined;
}
// Try the canonical key first, then any schema-aliased form.
if (Object.prototype.hasOwnProperty.call(parsed, canonicalKey)) {
return parsed[canonicalKey];
}
// Search for a key in the JSON that resolves to the same canonical key.
for (const jsonKey of Object.keys(parsed)) {
const resolved = resolveSchemaPropertyKey(jsonKey, schemaProperties, normalizedSchemaKeyMap, ambiguousNormalizedSchemaKeys);
if (resolved === canonicalKey) {
return parsed[jsonKey];
}
}
return undefined;
} catch {
return undefined;
}
}

/**
* Parse user-provided --key value pairs into a tool arguments object.
* Supports both --key value and --key=value styles.
Expand All @@ -634,10 +691,15 @@ function readStdinSync() {
* printf '{"issue_number":42,"body":"hello"}' | safeoutputs add_comment .
*
* When `stdinContent` is provided and non-empty, any '--key .' or '--key=.'
* pair substitutes that field's value with the raw stdin text (per-field
* stdin mode). This enables agents to pipe large text into a single field:
* printf 'Long issue body...' | safeoutputs create_issue --title "Bug" --body .
* When stdin is empty, the '.' is passed through as a literal value.
* pair substitutes that field's value with stdin. If stdin is a JSON object
* that contains the target key, the matching field value is extracted from the
* JSON instead of using the entire stdin string. This prevents a common agent
* mistake where the whole JSON payload ends up as a string field value:
* printf '{"title":"Fix bug","body":"Details"}' \
* | safeoutputs create_pull_request --title "Fix bug" --body .
* When stdin is empty, the '.' is passed through as a literal value. When
* stdin is non-empty but the key is absent from the JSON (or stdin is not a
* JSON object), the entire trimmed stdin string is used as the field value.
*
* @param {string[]} args - User arguments after the tool name
* @param {Record<string, {type?: string|string[]}>} [schemaProperties] - Tool input schema properties
Expand Down Expand Up @@ -686,7 +748,8 @@ function parseToolArgs(args, schemaProperties = {}, stdinContent = null) {
const canonicalKey = resolveSchemaPropertyKey(key, schemaProperties, normalizedSchemaKeyMap, ambiguousNormalizedSchemaKeys);
const rawValue = raw.slice(eqIdx + 1);
if (rawValue === "." && trimmedStdin) {
result[canonicalKey] = trimmedStdin;
const extracted = tryExtractJsonFieldFromStdin(trimmedStdin, canonicalKey, schemaProperties, normalizedSchemaKeyMap, ambiguousNormalizedSchemaKeys);
result[canonicalKey] = extracted !== undefined ? extracted : trimmedStdin;
} else {
result[canonicalKey] = coerceToolArgValue(canonicalKey, rawValue, schemaProperties[canonicalKey], result[canonicalKey], !hasSchemaProperties);
}
Expand All @@ -697,7 +760,8 @@ function parseToolArgs(args, schemaProperties = {}, stdinContent = null) {
const canonicalKey = resolveSchemaPropertyKey(raw, schemaProperties, normalizedSchemaKeyMap, ambiguousNormalizedSchemaKeys);
const rawValue = args[i + 1];
if (rawValue === "." && trimmedStdin) {
result[canonicalKey] = trimmedStdin;
const extracted = tryExtractJsonFieldFromStdin(trimmedStdin, canonicalKey, schemaProperties, normalizedSchemaKeyMap, ambiguousNormalizedSchemaKeys);
result[canonicalKey] = extracted !== undefined ? extracted : trimmedStdin;
} else {
result[canonicalKey] = coerceToolArgValue(canonicalKey, rawValue, schemaProperties[canonicalKey], result[canonicalKey], !hasSchemaProperties);
}
Expand Down Expand Up @@ -1441,6 +1505,7 @@ module.exports = {
parseToolArgs,
coerceToolArgValue,
unescapeCliStringArg,
tryExtractJsonFieldFromStdin,
extractJSONRPCMessages,
renderProgressMessages,
formatResponse,
Expand Down
157 changes: 157 additions & 0 deletions actions/setup/js/mcp_cli_bridge.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
shouldShowToolHelpForEmptyArgs,
showHelp,
showToolHelp,
tryExtractJsonFieldFromStdin,
unescapeCliStringArg,
writeStdoutAndFlush,
} from "./mcp_cli_bridge.cjs";
Expand Down Expand Up @@ -764,6 +765,162 @@ describe("mcp_cli_bridge.cjs", () => {
});
});

describe("per-field stdin mode with JSON stdin — field extraction", () => {
it("extracts matching field from JSON stdin when --body . is used (space-separated)", () => {
// Root cause of gh-aw-workshop#2118: agent piped JSON payload and used --body .
// expecting the body field to be extracted, but the entire JSON string ended up as body.
const schemaProperties = { title: { type: "string" }, body: { type: "string" } };
const stdinContent = '{"title":"Fix bug","body":"This PR fixes the issue."}';

const { args } = parseToolArgs(["--title", "Fix bug", "--body", "."], schemaProperties, stdinContent);

expect(args).toEqual({ title: "Fix bug", body: "This PR fixes the issue." });
});

it("extracts matching field from JSON stdin when --body=. is used (equals-separated)", () => {
const schemaProperties = { title: { type: "string" }, body: { type: "string" } };
const stdinContent = '{"title":"Fix bug","body":"Details here."}';

const { args } = parseToolArgs(["--title", "Fix bug", "--body=."], schemaProperties, stdinContent);

expect(args).toEqual({ title: "Fix bug", body: "Details here." });
});

it("extracts both title and body when --title . --body . used with JSON stdin", () => {
const schemaProperties = { title: { type: "string" }, body: { type: "string" } };
const stdinContent = '{"title":"Fix: Bug #123","body":"This PR fixes bug #123."}';

const { args } = parseToolArgs(["--title", ".", "--body", "."], schemaProperties, stdinContent);

expect(args).toEqual({ title: "Fix: Bug #123", body: "This PR fixes bug #123." });
});

it("falls back to raw stdin when JSON does not contain the target key", () => {
const schemaProperties = { body: { type: "string" } };
const stdinContent = '{"other_field":"value"}';

const { args } = parseToolArgs(["--body", "."], schemaProperties, stdinContent);

// No 'body' key in JSON → use raw stdin content
expect(args).toEqual({ body: stdinContent });
});

it("falls back to raw stdin when stdin is not a JSON object", () => {
const schemaProperties = { body: { type: "string" } };
const stdinContent = "This is a long body from stdin.";

const { args } = parseToolArgs(["--body", "."], schemaProperties, stdinContent);

expect(args).toEqual({ body: stdinContent });
});

it("falls back to raw stdin when stdin is a JSON array", () => {
const schemaProperties = { body: { type: "string" } };
const stdinContent = '["item1","item2"]';

const { args } = parseToolArgs(["--body", "."], schemaProperties, stdinContent);

expect(args).toEqual({ body: stdinContent });
});

it("falls back to raw stdin when stdin is invalid JSON", () => {
const schemaProperties = { body: { type: "string" } };
const stdinContent = '{"body": not-valid-json}';

const { args } = parseToolArgs(["--body", "."], schemaProperties, stdinContent);

expect(args).toEqual({ body: stdinContent });
});

it("resolves dash/underscore aliased JSON key to canonical schema key", () => {
const schemaProperties = { issue_number: { type: "integer" } };
const stdinContent = '{"issue-number":42}';

const { args } = parseToolArgs(["--issue_number", "."], schemaProperties, stdinContent);

expect(args).toEqual({ issue_number: 42 });
});

it("preserves non-string JSON values extracted from stdin (e.g. boolean, number)", () => {
const schemaProperties = { draft: { type: "boolean" }, count: { type: "integer" } };
const stdinContent = '{"draft":true,"count":5}';

const { args: draftArgs } = parseToolArgs(["--draft", "."], schemaProperties, stdinContent);
const { args: countArgs } = parseToolArgs(["--count", "."], schemaProperties, stdinContent);

expect(draftArgs).toEqual({ draft: true });
expect(countArgs).toEqual({ count: 5 });
});

it("handles multiline JSON payload with --body . correctly", () => {
const schemaProperties = { title: { type: "string" }, body: { type: "string" } };
const stdinContent = `{
"title": "Fix bug",
"body": "### Summary\\n\\nDetails here."
}`;

const { args } = parseToolArgs(["--title", "Fix bug", "--body", "."], schemaProperties, stdinContent);

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.

[/tdd] Missing edge case: null as an extracted JSON field value.

The extracted !== undefined guard is correct, but there is no test confirming that null (and by extension false, 0) flow through properly. A future refactor changing this guard to !extracted would silently break those values.

💡 Suggested test
it('preserves null value extracted from JSON stdin', () => {
  const schemaProperties = { body: { type: 'string' } };
  const { args } = parseToolArgs(['--body', '.'], schemaProperties, '{"body":null}');
  expect(args).toEqual({ body: null });
});

This pins the extracted !== undefined contract explicitly.

@copilot please address this.


expect(args).toEqual({ title: "Fix bug", body: "### Summary\n\nDetails here." });
});
});

describe("tryExtractJsonFieldFromStdin", () => {
it("extracts a field value from a JSON object string", () => {
const schemaProperties = { body: { type: "string" } };
const result = tryExtractJsonFieldFromStdin(
'{"title":"Fix","body":"PR description"}',
"body",
schemaProperties,
new Map([
["title", "title"],
["body", "body"],
]),
new Set()
);
expect(result).toBe("PR description");
});

it("returns undefined when the key is absent from the JSON", () => {
const schemaProperties = { body: { type: "string" } };
const result = tryExtractJsonFieldFromStdin('{"title":"Fix"}', "body", schemaProperties, new Map([["title", "title"]]), new Set());
expect(result).toBeUndefined();
});

it("returns undefined for non-object JSON (array)", () => {
const result = tryExtractJsonFieldFromStdin('["a","b"]', "body", {}, new Map(), new Set());
expect(result).toBeUndefined();
});

it("returns undefined for invalid JSON", () => {
const result = tryExtractJsonFieldFromStdin("{not valid}", "body", {}, new Map(), new Set());
expect(result).toBeUndefined();
});

it("returns undefined for plain text (not starting with {)", () => {
const result = tryExtractJsonFieldFromStdin("plain text", "body", {}, new Map(), new Set());
expect(result).toBeUndefined();
});

it("preserves null value extracted from JSON stdin (does not fall back to raw stdin)", () => {
const schemaProperties = { body: { type: "string" } };
const result = tryExtractJsonFieldFromStdin('{"body":null}', "body", schemaProperties, new Map([["body", "body"]]), new Set());
expect(result).toBeNull();
});

it("preserves false boolean extracted from JSON stdin", () => {
const schemaProperties = { draft: { type: "boolean" } };
const result = tryExtractJsonFieldFromStdin('{"draft":false}', "draft", schemaProperties, new Map([["draft", "draft"]]), new Set());
expect(result).toBe(false);
});

it("preserves 0 number extracted from JSON stdin", () => {
const schemaProperties = { count: { type: "number" } };
const result = tryExtractJsonFieldFromStdin('{"count":0}', "count", schemaProperties, new Map([["count", "count"]]), new Set());
expect(result).toBe(0);
});
});

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