refactor: extract shared MCP script handler execution envelope#29497
Merged
refactor: extract shared MCP script handler execution envelope#29497
Conversation
5 tasks
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b0f10b19-1533-4c55-af8c-4575d5caa4e2 Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Refactor duplicate MCP script handler execution envelope
refactor: extract shared MCP script handler execution envelope
May 1, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors MCP script handlers (Go/Python/JavaScript/Shell) to share a single process-execution “envelope” for consistent execFile behavior, logging, stdout parsing, and error enrichment, and adds targeted tests for the shared module and previously-untested handlers.
Changes:
- Added
mcp_handler_process.cjsto centralize process execution, enhanced errors, JSON parsing, and MCP response wrapping. - Updated Go/Python/JavaScript/Shell handlers to delegate execution to
executeProcess, preserving Shell-specific output-file parsing/cleanup via hooks. - Added new Vitest coverage for the shared execution helper and for Python/Shell handlers.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/mcp_handler_process.cjs | New shared execFile wrapper + enhanced error/message building + stdout JSON parsing + MCP content wrapping. |
| actions/setup/js/mcp_handler_process.test.cjs | Unit tests for enhanced errors, JSON parsing behavior, MCP wrapping, and executeProcess hooks. |
| actions/setup/js/mcp_handler_shell.cjs | Refactors shell handler to use executeProcess with onError cleanup and buildResult output-file parsing. |
| actions/setup/js/mcp_handler_shell.test.cjs | New tests covering env input mapping, output parsing, cleanup behavior, timeout, and cwd behavior for shell scripts. |
| actions/setup/js/mcp_handler_python.cjs | Refactors python handler to use executeProcess (stdin JSON, enhanced errors, parsing). |
| actions/setup/js/mcp_handler_python.test.cjs | New tests covering Python execution, JSON/non-JSON output behavior, timeouts, cwd behavior, and enhanced errors. |
| actions/setup/js/mcp_handler_javascript.cjs | Refactors JS handler to use executeProcess while keeping stdin JSON behavior. |
| actions/setup/js/mcp_handler_go.cjs | Refactors Go handler to use executeProcess while keeping stdin JSON behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 3
Comment on lines
+17
to
+32
| * @param {Error} error - The original execution error | ||
| * @param {string} scriptPath - Path to the script, used for context in the message | ||
| * @param {string} stdout - Process stdout output | ||
| * @param {string} stderr - Process stderr output | ||
| * @returns {Error} Enhanced error with stdout/stderr context | ||
| */ | ||
| function buildEnhancedError(error, scriptPath, stdout, stderr) { | ||
| const exitCode = typeof error.code === "number" ? error.code : 1; | ||
| const parts = [`Command failed: ${scriptPath} (exit code: ${exitCode})`]; | ||
| if (stderr && stderr.trim()) { | ||
| parts.push(`stderr:\n${stderr.trim()}`); | ||
| } | ||
| if (stdout && stdout.trim()) { | ||
| parts.push(`stdout:\n${stdout.trim()}`); | ||
| } | ||
| return new Error(parts.join("\n")); |
| if (error) { | ||
| server.debugError(` [${toolName}] ${languageLabel} script error: `, error); | ||
| if (onError) { | ||
| onError(error, stdout, stderr); |
Comment on lines
+128
to
+133
| if (buildResult) { | ||
| result = buildResult(stdout, stderr); | ||
| } else { | ||
| result = parseStdoutAsJson(stdout, stderr, () => { | ||
| server.debug(` [${toolName}] Output is not JSON, returning as text`); | ||
| }); |
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification DetailsView all 44 test classifications
Flagged Tests — Requires ReviewNo tests are flagged. All 44 tests verify behavioral contracts. Inflation NoteThe test inflation penalty (10 pts) was applied because two handler files show a >2:1 ratio of test lines added vs. production lines added:
Context: This is a refactoring PR that extracted shared execution logic into Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §25210794262
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Collaborator
|
@copilot review all |
…t hooks Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7848f82c-7265-4814-9346-9565909c2241 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Addressed all three review findings in df0e346:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Go, Python, JavaScript, and Shell MCP script handlers each duplicated ~70 lines of identical process execution logic —
execFilesetup, stdout/stderr logging, timeout/maxBuffer, JSON parsing, and MCP content wrapping. Error handling had already diverged: JS and Shell produced enhanced errors with stdout/stderr context; Go and Python forwarded the raw error.Changes
New
mcp_handler_process.cjs— shared execution envelope exposing:executeProcess(opts)— commonexecFilewrapper withonError(cleanup hook) andbuildResult(custom result mapper) extension pointsbuildEnhancedError(error, scriptPath, stdout, stderr)— informative errors with stdout/stderr contextparseStdoutAsJson(stdout, stderr, onParseFailure)— JSON parse with text fallbackwrapMCPContent(result)— MCPcontentresponse constructionRefactored handlers — all four handlers delegate to
executeProcess; Go and Python error handling normalized to enhanced errors, matching JS/ShellShell-specific extensions preserved —
onErrorhandles GITHUB_OUTPUT file cleanup;buildResulthandles key=value output parsing;cleanupOutputFilepromoted to module-levelNew tests —
mcp_handler_process.test.cjs(26 tests),mcp_handler_python.test.cjs(7 tests, previously missing),mcp_handler_shell.test.cjs(11 tests, previously missing)