[Copilot]

The MCP CLI bridge (safeoutputs add_comment --body "...") silently drops body content after the first line because the shell splits unquoted multiline strings into separate argv tokens, and parseToolArgs only consumes one token per --key.

Changes

• mcp_cli_bridge.cjs

  • hasStdinPlaceholder(args) — scans CLI args for - placeholder values in both --key - and --key=- forms; includes type guard for non-string entries
  • readStdinSync() — synchronously drains stdin via fs.readSync(0, ...) with a 10 MB hard cap to prevent memory exhaustion
  • parseToolArgs(args, schemaProperties, stdinContent) — when stdinContent is provided, substitutes it wherever a value is exactly -
  • main() — calls hasStdinPlaceholder before arg parsing and pre-reads stdin once if needed

• mcp_cli_tools_prompt.md — documents the stdin piping pattern for agents so they use it for multiline content

Usage

# Replaces fragile command-substitution quoting:
#   safeoutputs add_comment --body "$(cat body.txt)"  ← newlines can be mangled

# Use stdin piping instead:
printf '### Title\n\nFull body here.' | safeoutputs add_comment --body -

# --key=- form also supported:
cat report.md | safeoutputs add_comment --body=-

[Copilot]

Pull request overview

Adds stdin piping support to the MCP CLI bridge so multiline tool arguments (e.g., --body) can be provided reliably via --key - / --key=- without shell-quoting issues.

Changes:

• Add detection for - stdin placeholders and synchronously pre-read stdin once when needed.
• Extend parseToolArgs to substitute stdin content for placeholder values.
• Document the stdin piping pattern and add test coverage for placeholder detection/substitution and stdin size limits.

Show a summary per file

File	Description
actions/setup/js/mcp_cli_bridge.cjs	Implements stdin placeholder detection, stdin draining with size cap, and argument substitution.
actions/setup/js/mcp_cli_bridge.test.cjs	Adds unit tests covering placeholder parsing, detection, and max-size enforcement behavior.
actions/setup/md/mcp_cli_tools_prompt.md	Documents the recommended stdin piping usage for multiline CLI arguments.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 3/3 changed files
• Comments generated: 1

[Copilot]

readStdinSync swallows all fs.readSync errors and just breaks the loop, which can silently return truncated/partial stdin if a read fails after some bytes have already been collected. This also conflicts with the docstring claim that it “Returns an empty string if reading fails”. Consider either (a) rethrowing non-EOF errors (or at least when some bytes were already read), or (b) explicitly returning "" on any read error and documenting that behavior, so callers don’t unknowingly send partial content.

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 89/100

✅ Excellent

Metric	Value
New/modified tests analyzed	11
✅ Design tests (behavioral contracts)	11 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	7 (64%)
Duplicate test clusters	0
Test inflation detected	No (1.18:1 ratio)
🚨 Coding-guideline violations	None

---

Test Classification Details

View all 11 test classifications

Test	File	Classification	Issues Detected
substitutes stdin content for '-' value in --key value form	mcp_cli_bridge.test.cjs	✅ Design	Happy path; verifies correct substitution
substitutes stdin content for '-' value in --key=value form	mcp_cli_bridge.test.cjs	✅ Design	Edge: alternate --key=value syntax
does not substitute '-' when stdinContent is null	mcp_cli_bridge.test.cjs	✅ Design	Edge: null stdin guard
substitutes stdin only for the '-' placeholder, leaves other args unchanged	mcp_cli_bridge.test.cjs	✅ Design	Edge: mixed args isolation
uses same stdinContent for multiple '-' placeholders	mcp_cli_bridge.test.cjs	✅ Design	Edge: multiple placeholders
detects stdin placeholder in --key value form	mcp_cli_bridge.test.cjs	✅ Design	Happy path for hasStdinPlaceholder
detects stdin placeholder in --key=value form	mcp_cli_bridge.test.cjs	✅ Design	Edge: alternate syntax detection
returns false when no stdin placeholder is present	mcp_cli_bridge.test.cjs	✅ Design	Negative case
returns false for empty args	mcp_cli_bridge.test.cjs	✅ Design	Edge: empty input
does not treat '-' after a non-flag arg as a placeholder	mcp_cli_bridge.test.cjs	✅ Design	Edge: positional - boundary case
throws when stdin exceeds maximum allowed size	mcp_cli_bridge.test.cjs	✅ Design	Error path: DoS protection

---

Flagged Tests — Requires Review

No tests flagged. All 11 new tests verify observable behavioral contracts.

---

Language Support

Tests analyzed:

• 🐹 Go (*_test.go): 0 tests
• 🟨 JavaScript (*.test.cjs, *.test.js): 11 tests (vitest)

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). All 11 tests assert on observable outputs — return values, argument parsing results, and thrown errors. The single vi.spyOn(fs, "readSync") mock targets external I/O, which is an acceptable mocking pattern. Error/edge cases are well covered: null input, empty args, oversized stdin (security boundary), positional-- disambiguation, and the negative detection case.

---

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References: §25203857911

🧪 Test quality analysis by Test Quality Sentinel · ● 480.7K · ◷

[github-actions]

✅ Test Quality Sentinel: 89/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%).

[pelikhan]

@copilot review all comments

[Copilot]

@copilot review all comments

Fixed in 8c1319c. readStdinSync now rethrows any error that occurs after bytes have already been collected (preventing silent partial content), and returns "" only when the error happens before any bytes are read (e.g. stdin not connected). Added two new tests covering both cases.