fix(mcp-cli): per-field stdin mode extracts JSON field instead of using raw JSON string as value - #48100
Conversation
…ent raw JSON being used as PR body Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions in default business dirs). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Updates MCP CLI per-field stdin handling to extract matching values from JSON objects.
Changes:
- Adds JSON field extraction with key alias support.
- Preserves extracted JSON value types.
- Adds unit and integration-style parsing tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/mcp_cli_bridge.cjs |
Implements JSON field extraction for per-field stdin. |
actions/setup/js/mcp_cli_bridge.test.cjs |
Tests extraction, aliases, types, and fallbacks. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/mcp_cli_bridge.cjs:747
- This content sniffing changes the established
--key .contract from “raw text for that field” (documented at line 558 and covered by the existing per-field tests). A legitimate JSON document can now be silently corrupted: piping{"body":"literal"}to--body .sends onlyliteral, so there is no way to submit that JSON text unchanged through per-field stdin. Keep per-field stdin raw and require the existing whole-payload.mode for JSON extraction, or introduce an explicit opt-in mode rather than inferring semantics from the content.
const extracted = tryExtractJsonFieldFromStdin(trimmedStdin, canonicalKey, schemaProperties, normalizedSchemaKeyMap, ambiguousNormalizedSchemaKeys);
result[canonicalKey] = extracted !== undefined ? extracted : trimmedStdin;
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| * When stdin is empty or the key is absent from the JSON, the '.' is passed | ||
| * through as a literal value. |
There was a problem hiding this comment.
The fix is correct and well-tested. tryExtractJsonFieldFromStdin properly handles all edge cases (non-object JSON, missing key, invalid JSON, leading-whitespace-trimmed input) and falls back to raw stdin. Both --key . and --key=. forms are covered. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 10.4 AIC · ⌖ 7.3 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Root cause is correctly addressed: the fix extracts the JSON field value rather than patching the symptom at call sites.
- Fallback semantics are sound: every non-JSON, invalid-JSON, or key-absent path returns
undefinedand falls through to raw stdin — preserving all existing behaviour. - Test coverage is thorough: the new suite covers both the
parseToolArgsintegration path and thetryExtractJsonFieldFromStdinunit directly, including dash/underscore aliasing and non-string types.
Minor gaps (see inline comments)
nullvalue not tested —extracted !== undefinedis the right guard, but there's no test that anullvalue in the JSON flows through correctly. A future refactor to a falsy check would silently break it.startsWith('{')assumes pre-trimmed input — documented as a precondition, but worth a comment or assertion so future callers don't miss it.
Positive Highlights
- ✅ Clean single-responsibility helper with narrow, well-documented interface
- ✅ Exported and directly unit-tested — good for future maintenance
- ✅ Both
--key .and--key=.forms covered - ✅ Non-string type preservation (boolean, number) explicitly tested
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 24.7 AIC · ⌖ 4.77 AIC · ⊞ 6.7K
Comment /matt to run again
| "body": "### Summary\\n\\nDetails here." | ||
| }`; | ||
|
|
||
| const { args } = parseToolArgs(["--title", "Fix bug", "--body", "."], schemaProperties, stdinContent); |
There was a problem hiding this comment.
[/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.
| * 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 . |
There was a problem hiding this comment.
[/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.
🧪 Test Quality Sentinel Report - PR #48100✅ Test Quality Score: 86/100 — Excellent 15 tests analyzed (all JS). 15 design tests, 0 implementation tests, 0 violations. Score breakdown: 40 (100% design) + 26 (87% edge coverage) + 20 (0 dup clusters) + 0 (inflation 2.1:1) = 86. Verdict: PASSED. 0% implementation tests (threshold 30%). Tests cover the root-cause bug scenario and all edge/fallback cases including: missing key, invalid JSON, JSON array, plain text, type preservation, and dash/underscore aliasing.
|
There was a problem hiding this comment.
REQUEST_CHANGES — Two correctness issues that need fixing before merge.
Blocking issues
-
** JSON values bypass fallback** (line 46/90):
hasOwnPropertyreturnstruefornull-valued fields, soextractedisnullwhich passes the!== undefinedcheck. A stdin like{"body": null}silently injectsnullinto a string field instead of falling back to the raw JSON string. Fix: also checkextracted !== null, or document thatnullextraction is intentional. -
Extracted values skip
coerceToolArgValue(line 90/100): The extraction path assigns values directly, bypassing the coercion that normalizes types (e.g."true"→true,"42"→42). This creates asymmetric behavior between--flag valueand--flag .with equivalent JSON. Should pipe extracted values throughcoerceToolArgValueto stay consistent.
Non-blocking
- Aliased key resolution is first-match-wins with no warning (line 54): when stdin has both
issue_numberandissue-number, the result depends on key enumeration order. Document this as the caller's responsibility.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 46.7 AIC · ⌖ 4.99 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/mcp_cli_bridge.cjs:46
null JSON field values bypass the undefined guard and are injected silently: hasOwnProperty returns true for null values, so extracted becomes null, which satisfies extracted !== undefined, and the null is assigned directly — skipping the raw-stdin fallback.
<details>
<summary>💡 Suggested fix</summary>
Given stdin {"body": null} and --body ., the result is body: null instead of body: '{"body":null}' (the full raw string). Downstream MCP calls that expect a string …
actions/setup/js/mcp_cli_bridge.cjs:90
Extracted values skip coerceToolArgValue, breaking schema type coercion for all non-string types: the existing path calls coerceToolArgValue to normalize values (e.g. coerce "true" → true, "42" → 42), but the extracted-JSON path bypasses it entirely.
<details>
<summary>💡 Detail and impact</summary>
This creates an asymmetry: if an agent passes --draft true the value goes through coercion; if they pass {"draft":true} via stdin with --draft ., it also works (JSON already …
actions/setup/js/mcp_cli_bridge.cjs:54
First-match-wins on aliased JSON keys is non-deterministic when stdin has duplicate aliases: the loop over Object.keys(parsed) stops at the first key that resolves to canonicalKey. If stdin JSON contains both issue_number and issue-number (both aliasing to the same canonical key), whichever JS enumerates first wins — and JS object key enumeration order, while insertion-order in V8, is not guaranteed by spec for non-integer keys across all engines.
<details>
<summary>💡 Detail</summ…
|
@copilot please run the Unresolved review feedback:
|
…ield stdin mode Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed the inaccurate JSDoc fallback description in
Commit: fix(mcp-cli): correct parseToolArgs JSDoc fallback contract for per-field stdin mode |
|
@copilot please run the Unresolved review feedback:
|
…ent trimmedStdin precondition Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All three unresolved threads addressed:
Commit: test(mcp-cli): pin extracted null/false/0 value contracts; doc: document trimmedStdin precondition |
🤖 PR Triage — §30192186368
Why: Per-field stdin mode used entire JSON payload as field value instead of extracting the field — causing PR bodies to contain raw JSON strings. Bot-approved. CI status unknown; awaits full run. Next: Verify CI passes, then fast-track merge.
|
|
🎉 This pull request is included in a new release. Release: |
When an agent piped a full JSON payload but used per-field stdin mode (
--body .), the entire JSON string was assigned as the field value — causing PR bodies to contain raw unparsed JSON.Root cause
create_pull_requesthas alabels: arrayfield, soshouldUseJsonModereturnstrueand the CLI help promotes JSON payload mode. Agents can mistakenly mix the two modes: pipe{"title":"...","body":"..."}while passing--body ., which in per-field mode setsbody = trimmedStdin(the whole JSON string).Fix
Added
tryExtractJsonFieldFromStdin()inmcp_cli_bridge.cjs. In per-field mode, when stdin is a valid JSON object that contains the target key, the value is extracted from the JSON rather than using raw stdin. Falls back to raw stdin when stdin is not JSON or lacks the key — preserving all existing behavior.Handles dash/underscore key aliasing and preserves non-string value types. Both
--key .and--key=.forms are covered.run: https://github.com/github/gh-aw/actions/runs/30187302401