From ccafe55e5cf3d79720df70efb6a336901c1df21f Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Thu, 1 Oct 2026 23:52:33 +0200 Subject: [PATCH 1/2] fix(github-mcp): vendor bash-mcp-sdk v5.2.0 and drop cancelled partials shared/mcpserver_core.sh is vendored from shopwareLabs/bash-mcp-sdk v5.2.0, up from v5.0.0. v5.2.0 dispatches a tools/call only when the running server's tools list declares the tool and answers -32601 otherwise, which is the guarantee _gh_unset_undeclared_tools added in 4.2.0. The protocol layer also covers two routes the helper did not: an executable named tool_ on PATH, and a tool__cancel hook called as a tool. The helper is removed. tool_dispatch_declaration.bats passes unchanged against the SDK's enforcement, so the read server and the disabled write server still answer "Tool not found" for every tool they do not declare. v5.1.0 chains an EXIT trap set before run_mcp_server into teardown. Neither server sets one, and the comment in server-write.sh that said such a trap never runs now gives only the reasons that still hold. Removing the helper also removed the jq read it did over the tools list at startup. The SDK reads that list only per tools/list or tools/call request, so a missing or corrupt tools-read.json let the server start and then fail every such call. _gh_require_tools_list restores the refusal at startup in both servers and applies the SDK's own rule that the file holds exactly one JSON object. A cancelled repo_file or search_code download with download_to left its .partial.XXXXXX sibling next to the user's file. Cancellation sends SIGTERM to the call's process group and nothing removed the file. Both sites now go through _gh_partial_create, which installs a fixed EXIT trap before the file exists and creates it with noclobber in the tool's own shell. mktemp created the file inside a $() subshell before the caller could name it in a trap. The name is recorded only after creation, so the trap never removes a file another process left under a colliding name. No earlier EXIT handler is captured or chained. In a subshell with no trap of its own, trap -p reports the parent's handler, which here is the server's _server_teardown, and running that inside a tool shell would tear the server down. A SIGTERM on the single assignment after creation, or a SIGKILL, still leaves the sibling behind. The partial file now takes its mode from umask instead of mktemp's 0600. _gh_download_file redirected gh's stderr into the downloaded file with 2>&1, so a warning on an otherwise successful search_code download became part of the file. stderr is now captured apart from the body. A warning on success is logged, and a failure's text is returned in the error message. Released as 4.2.1. Every tool a server declares behaves as before. Co-Authored-By: Claude Opus 5.5 (1M context) --- .mcp-sdk.lock | 4 +- .../github-mcp/download_cancel_cleanup.bats | 150 +++++ plugin-tests/github-mcp/mcp_tool_gh.bats | 59 ++ .../github-mcp/server_startup_tools_list.bats | 60 ++ .../github-mcp/tool_dispatch_declaration.bats | 10 +- plugins/github-mcp/.claude-plugin/plugin.json | 2 +- plugins/github-mcp/.codex-plugin/plugin.json | 2 +- plugins/github-mcp/CHANGELOG.md | 12 + .../github-mcp/mcp-server-gh/lib/common.sh | 176 +++-- plugins/github-mcp/mcp-server-gh/lib/repo.sh | 15 +- .../github-mcp/mcp-server-gh/server-read.sh | 2 +- .../github-mcp/mcp-server-gh/server-write.sh | 8 +- plugins/github-mcp/shared/mcpserver_core.sh | 600 +++++++++++++++--- 13 files changed, 933 insertions(+), 167 deletions(-) create mode 100644 plugin-tests/github-mcp/download_cancel_cleanup.bats create mode 100644 plugin-tests/github-mcp/server_startup_tools_list.bats diff --git a/.mcp-sdk.lock b/.mcp-sdk.lock index c38bc9a..611e1e0 100644 --- a/.mcp-sdk.lock +++ b/.mcp-sdk.lock @@ -1,2 +1,2 @@ -version=v5.0.0 -sha256=b8e55b1f05fe64d165412a5bedc8b5baba764812e13ef0821a536a6c58b7cf07 +version=v5.2.0 +sha256=b58b62d82c61f96bc279971afa6c86fb84bd2f8635ac3c6c07bf6ddf6faa727f diff --git a/plugin-tests/github-mcp/download_cancel_cleanup.bats b/plugin-tests/github-mcp/download_cancel_cleanup.bats new file mode 100644 index 0000000..a6a39cf --- /dev/null +++ b/plugin-tests/github-mcp/download_cancel_cleanup.bats @@ -0,0 +1,150 @@ +#!/usr/bin/env bats +# bats file_tags=github-mcp,mcp-tools,cancellation +# A cancelled download leaves nothing next to the destination. +# +# repo_file and search_code write to a `.partial.*` sibling and rename it +# once the body is complete. The protocol layer cancels a call by sending +# SIGTERM to the call's process group; afterwards the destination directory +# holds only files whose download finished. +bats_require_minimum_version 1.11.0 + +load 'test_helper/common_setup' + +setup() { + log() { :; } + + GH_DEFAULT_REPO="shopware/shopware" + GH_TOOLING_CONFIG_FILE="" + + source "${GH_LIB_DIR}/common.sh" + source "${GH_LIB_DIR}/search.sh" + source "${GH_LIB_DIR}/repo.sh" + + DL_DIR="${BATS_TEST_TMPDIR}/dl" + mkdir -p "${DL_DIR}" + + # The search answers with one match; a file download writes part of a body + # and then blocks, so the call is still mid-write when it is cancelled. + gh() { + if [[ "$1" == "api" && "$2" == "--help" ]]; then + return 0 + fi + if [[ "$1" == "search" ]]; then + printf '%s\n' '[{"repository":{"nameWithOwner":"shopware/shopware"},"path":"composer.json"}]' + return 0 + fi + printf '%s' '{"name": "shopware/' + sleep 30 + } +} + +# Run a tool the way the protocol layer does — in a background subshell leading +# its own process group, stdin closed — and set CALL_PID to the group's pid. +start_tool_call() { + local tool_fn="$1" args="$2" + set -m + ( set +e; "${tool_fn}" "${args}" ) > "${BATS_TEST_TMPDIR}/call-output" 2>&1 < /dev/null & + CALL_PID=$! + set +m +} + +# Wait up to 5 seconds for a non-empty partial file under the given directory, +# optionally one whose name starts with the given destination file name. +wait_for_partial_body() { + local dir="$1" name="${2:-}" waited=0 + while [[ ${waited} -lt 50 ]]; do + if find "${dir}" -name "${name}*.partial.*" -size +0 | grep -q .; then + return 0 + fi + sleep 0.1 + waited=$((waited + 1)) + done + fail "no partial download appeared under ${dir} within 5 seconds" +} + +# Send SIGTERM to the call's process group and wait up to 5 seconds for it to +# end. A group still alive after that is killed so the test cannot hang. +cancel_tool_call() { + local pid="$1" waited=0 + kill -TERM -- "-${pid}" + while kill -0 -- "-${pid}" 2>/dev/null && [[ ${waited} -lt 50 ]]; do + sleep 0.1 + waited=$((waited + 1)) + done + if kill -0 -- "-${pid}" 2>/dev/null; then + kill -KILL -- "-${pid}" 2>/dev/null || true + wait "${pid}" || true + fail "the cancelled call was still running 5 seconds after SIGTERM" + fi + wait "${pid}" || true +} + +@test "a cancelled repo_file download removes its partial file" { + local dest="${DL_DIR}/composer.json" + start_tool_call tool_repo_file '{"repository":"shopware/shopware","path":"composer.json","download_to":"'"${dest}"'"}' + wait_for_partial_body "${DL_DIR}" + + cancel_tool_call "${CALL_PID}" + + run find "${DL_DIR}" -name '*.partial.*' + assert_output "" + assert [ ! -e "${dest}" ] +} + +@test "a cancelled search_code download removes its partial file" { + local dest="${DL_DIR}/shopware/shopware/composer.json" + start_tool_call tool_search_code '{"search":"name","download_to":"'"${DL_DIR}"'"}' + wait_for_partial_body "${DL_DIR}" + + cancel_tool_call "${CALL_PID}" + + run find "${DL_DIR}" -name '*.partial.*' + assert_output "" + assert [ ! -e "${dest}" ] +} + +@test "a search_code download cancelled on its second file keeps the first and drops the second" { + local dest_a="${DL_DIR}/shopware/shopware/a.json" + local dest_b="${DL_DIR}/shopware/shopware/b.json" + + # Two matches: the first download completes, the second blocks mid-write. + # The cancel waits for the second file's partial body, so it lands there. + gh() { + if [[ "$1" == "api" && "$2" == "--help" ]]; then + return 0 + fi + if [[ "$1" == "search" ]]; then + printf '%s\n' '[{"repository":{"nameWithOwner":"shopware/shopware"},"path":"a.json"},{"repository":{"nameWithOwner":"shopware/shopware"},"path":"b.json"}]' + return 0 + fi + if [[ "$*" == *"a.json"* ]]; then + printf '%s' '{"first":"complete"}' + return 0 + fi + printf '%s' '{"second":"incomplete' + sleep 30 + } + + start_tool_call tool_search_code '{"search":"name","download_to":"'"${DL_DIR}"'"}' + wait_for_partial_body "${DL_DIR}" "b.json" + + cancel_tool_call "${CALL_PID}" + + run find "${DL_DIR}" -name '*.partial.*' + assert_output "" + assert [ -e "${dest_a}" ] + assert_equal "$(cat "${dest_a}")" '{"first":"complete"}' + assert [ ! -e "${dest_b}" ] +} + +@test "a cancelled repo_file download with suppress_errors leaves no partial or destination" { + local dest="${DL_DIR}/composer.json" + start_tool_call tool_repo_file '{"repository":"shopware/shopware","path":"composer.json","download_to":"'"${dest}"'","suppress_errors":true}' + wait_for_partial_body "${DL_DIR}" + + cancel_tool_call "${CALL_PID}" + + run find "${DL_DIR}" -name '*.partial.*' + assert_output "" + assert [ ! -e "${dest}" ] +} diff --git a/plugin-tests/github-mcp/mcp_tool_gh.bats b/plugin-tests/github-mcp/mcp_tool_gh.bats index 0e2154a..8373b14 100644 --- a/plugin-tests/github-mcp/mcp_tool_gh.bats +++ b/plugin-tests/github-mcp/mcp_tool_gh.bats @@ -1065,6 +1065,65 @@ bats_test_function --description "pr_checks: fails without repo outside git" -- assert_output "not available" } +# ============================================================================= +# _gh_download_file — gh's own diagnostics stay out of the downloaded file +# (used by search_code's download_to loop; tool_repo_file has its own +# separate download path, covered above) +# ============================================================================= + +@test "_gh_download_file: a stderr warning on a successful download is logged, not written into the file" { + local dl_path="${BATS_TEST_TMPDIR}/dl_warn/composer.json" + local warned="" + log() { [[ "$1" == "WARN" ]] && warned="$2"; } + gh() { + [[ "$1" == "api" && "$2" == "--help" ]] && return 0 + printf 'file body' + printf 'gh: a warning\n' >&2 + } + _gh_download_file "shopware" "shopware" "composer.json" "${dl_path}" + [[ "$(cat "${dl_path}")" == "file body" ]] || { + echo "Expected only the body in the file, got: $(cat "${dl_path}")" + return 1 + } + [[ "${warned}" == *"a warning"* ]] || { + echo "Expected the warning logged, got: ${warned}" + return 1 + } +} + +@test "_gh_download_file: a failing gh reports its stderr and leaves nothing behind" { + local dl_path="${BATS_TEST_TMPDIR}/dl_fail/composer.json" + local out_file="${BATS_TEST_TMPDIR}/download_output" + gh() { + [[ "$1" == "api" && "$2" == "--help" ]] && return 0 + printf 'gh: rate limit exceeded\n' >&2 + return 1 + } + # Called directly with plain redirection rather than via `run` (a command + # substitution, which forks a subshell): _gh_partial_create/_finish read + # and restore this shell's own EXIT trap, and bats sets one of its own for + # the real test process, so a forked copy re-arming it fires a spurious + # second report when that copy's subshell ends. + local status=0 + _gh_download_file "shopware" "shopware" "composer.json" "${dl_path}" > "${out_file}" 2>&1 || status=$? + [[ ${status} -ne 0 ]] || { + echo "Expected failure, got success: $(cat "${out_file}")" + return 1 + } + grep -q "rate limit exceeded" "${out_file}" || { + echo "Expected error text, got: $(cat "${out_file}")" + return 1 + } + [[ ! -e "${dl_path}" ]] || { + echo "Destination should not exist: ${dl_path}" + return 1 + } + ! compgen -G "${dl_path}.partial.*" >/dev/null || { + echo "Partial file left behind" + return 1 + } +} + # ============================================================================= # run_list — new filter params # ============================================================================= diff --git a/plugin-tests/github-mcp/server_startup_tools_list.bats b/plugin-tests/github-mcp/server_startup_tools_list.bats new file mode 100644 index 0000000..6aac134 --- /dev/null +++ b/plugin-tests/github-mcp/server_startup_tools_list.bats @@ -0,0 +1,60 @@ +#!/usr/bin/env bats +# bats file_tags=github-mcp,startup +# A missing or unreadable tools list stops the server at startup. +# +# mcpserver_core.sh reads MCP_TOOLS_LIST_FILE lazily, once per tools/list or +# tools/call request, so nothing else catches a corrupt or missing list before +# the first call — the server would otherwise start cleanly and then fail +# every tools/list and tools/call request it receives. +bats_require_minimum_version 1.11.0 + +load 'test_helper/common_setup' + +setup() { + # A private copy: these tests corrupt tools-read.json, and the real + # plugin directory is shared with every other suite in this run. + PLUGIN_COPY="${BATS_TEST_TMPDIR}/plugin" + cp -R "${PLUGIN_DIR}" "${PLUGIN_COPY}" + PROJECT_DIR="${BATS_TEST_TMPDIR}/project" + mkdir -p "${PROJECT_DIR}" +} + +# Pipe one initialize request into the copied read server and capture both +# streams plus the exit status. +run_server() { + printf '{"jsonrpc":"2.0","id":1,"method":"initialize","params":{}}\n' \ + | env PROJECT_ROOT="${PROJECT_DIR}" bash "${PLUGIN_COPY}/mcp-server-gh/server-read.sh" +} + +@test "server-read.sh refuses to start when tools-read.json is invalid JSON" { + printf '%s\n' '{not valid json' > "${PLUGIN_COPY}/mcp-server-gh/tools-read.json" + + run run_server + assert_failure + refute_output --partial '"jsonrpc"' + assert_output --partial "tools-read.json" +} + +@test "server-read.sh refuses to start when tools-read.json holds more than one JSON document" { + printf '%s\n%s\n' '{"tools":[]}' '{"tools":[]}' > "${PLUGIN_COPY}/mcp-server-gh/tools-read.json" + + run run_server + assert_failure + refute_output --partial '"jsonrpc"' + assert_output --partial "tools-read.json" +} + +@test "server-read.sh refuses to start when tools-read.json is missing" { + rm -f "${PLUGIN_COPY}/mcp-server-gh/tools-read.json" + + run run_server + assert_failure + refute_output --partial '"jsonrpc"' + assert_output --partial "tools-read.json" +} + +@test "server-read.sh still answers initialize when tools-read.json is valid" { + run run_server + assert_success + assert_output --partial '"jsonrpc"' +} diff --git a/plugin-tests/github-mcp/tool_dispatch_declaration.bats b/plugin-tests/github-mcp/tool_dispatch_declaration.bats index 529949a..57e852b 100644 --- a/plugin-tests/github-mcp/tool_dispatch_declaration.bats +++ b/plugin-tests/github-mcp/tool_dispatch_declaration.bats @@ -3,11 +3,11 @@ # A server must run only the tools its own tools list declares. # # api.sh, label.sh, and project.sh are shared by both servers and each carries -# tools the other does not declare. Dispatch resolves a tools/call to a shell -# function by name, so without _gh_unset_undeclared_tools every sourced tool is -# callable — which put the write-side label_add, label_remove, project_item_add, -# project_status_set, and api on the always-active read server, each running -# with no schema to validate its arguments against. +# tools the other does not declare. The protocol layer (bash-mcp-sdk v5.2.0+) +# refuses a tool the running server's list does not declare, which keeps the +# write-side label_add, label_remove, project_item_add, project_status_set, and +# api off the always-active read server. These tests pin that guarantee for the +# shared libs. bats_require_minimum_version 1.11.0 load 'test_helper/common_setup' diff --git a/plugins/github-mcp/.claude-plugin/plugin.json b/plugins/github-mcp/.claude-plugin/plugin.json index 884e34b..e2cd229 100644 --- a/plugins/github-mcp/.claude-plugin/plugin.json +++ b/plugins/github-mcp/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "github-mcp", - "version": "4.2.0", + "version": "4.2.1", "description": "GitHub CLI MCP servers wrapping gh for pull requests, issues, CI runs, jobs, commits, search, labels, projects, and reviews. Read server (always active) with 31 tools and write server (opt-in) with 25 tools. Includes SessionStart hook that injects MCP tool directives and PreToolUse hooks that enforce MCP tool usage. Configuration-optional: works without config when gh is authenticated.", "author": { "name": "Shopware Labs" diff --git a/plugins/github-mcp/.codex-plugin/plugin.json b/plugins/github-mcp/.codex-plugin/plugin.json index 924031a..651666b 100644 --- a/plugins/github-mcp/.codex-plugin/plugin.json +++ b/plugins/github-mcp/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "github-mcp", - "version": "4.2.0", + "version": "4.2.1", "description": "GitHub CLI MCP servers for pull requests, issues, CI, repositories, and other GitHub workflows, with shared lifecycle hooks that steer agents to the MCP tools.", "author": { "name": "Shopware Labs" diff --git a/plugins/github-mcp/CHANGELOG.md b/plugins/github-mcp/CHANGELOG.md index 8e7711f..1ab6de9 100644 --- a/plugins/github-mcp/CHANGELOG.md +++ b/plugins/github-mcp/CHANGELOG.md @@ -5,6 +5,18 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [4.2.1] - 2026-10-01 + +### Fixed + +- A cancelled `repo_file` or `search_code` download with `download_to` no longer leaves its `.partial.*` sibling next to the destination. The file is now created with `noclobber` in the tool's own shell rather than via `mktemp`, whose `$()` subshell created the file well before anything could name it in a trap, and an `EXIT` trap installed before the file exists removes it on the SIGTERM a cancelled call's process group receives. Two cases still leave an empty or partial sibling: a SIGTERM landing on the single assignment that records the new file's name, and a call killed with SIGKILL. The partial file's mode now comes from umask rather than `mktemp`'s fixed `0600`, so the renamed destination ends with an ordinary file mode. +- `search_code` with `download_to` no longer writes gh's own stderr into a downloaded file. `_gh_download_file` redirected gh's diagnostics into the destination alongside its body, so a warning on an otherwise successful download became part of the file's contents; stderr is now captured apart from the body, a warning on success is logged instead, and a failure's captured text is returned in the tool's error message. `repo_file`'s own download path already separated them and is unaffected. +- A missing or unreadable tools list stops the server at startup again. Removing `_gh_unset_undeclared_tools` (see Changed, below) also removed the startup `jq` read it incidentally did over the tools list; the protocol layer reads that list lazily, once per `tools/list` or `tools/call` request, so nothing else caught a corrupt or absent list before the first call, and the server started cleanly only to fail every `tools/list` and `tools/call` request. A dedicated startup check restores the refusal, naming the file, and applies the protocol layer's own rule that the file holds exactly one JSON object. + +### Changed + +- `shared/mcpserver_core.sh` is vendored from [shopwareLabs/bash-mcp-sdk](https://github.com/shopwareLabs/bash-mcp-sdk) `v5.2.0`, up from `v5.0.0`. The restriction to declared tools now lives in the protocol layer, which also stops an executable named `tool_` on `PATH` and a `tool__cancel` hook from being called as a tool, so `_gh_unset_undeclared_tools` is removed. v5.1.0 chains an `EXIT` trap set before the server starts into its teardown; these servers set none, so that change does not reach them. Every tool a server declares is unaffected. + ## [4.2.0] - 2026-09-13 ### Security diff --git a/plugins/github-mcp/mcp-server-gh/lib/common.sh b/plugins/github-mcp/mcp-server-gh/lib/common.sh index e19603f..e9a5ab2 100644 --- a/plugins/github-mcp/mcp-server-gh/lib/common.sh +++ b/plugins/github-mcp/mcp-server-gh/lib/common.sh @@ -36,37 +36,6 @@ _gh_validate_sha() { fi } -# Restrict dispatch to the tools this server declares. -# -# The protocol layer resolves a tools/call to a shell function by name, so every -# sourced tool_* function is callable whether or not this server's tools list -# declares it. api.sh, label.sh, and project.sh are shared by both servers and -# each carries tools the other does not declare, which put the write-side -# label_add, label_remove, project_item_add, project_status_set, and api on the -# always-active read server. An undeclared tool also has no schema, and argument -# validation treats a missing schema as nothing to check, so those tools ran -# with their arguments unvalidated. -# -# Dropping the undeclared functions makes the tools list the only thing that -# decides what this server runs. An empty tools list therefore leaves no tool -# callable, which is how the write server stays inert until it is enabled. -_gh_unset_undeclared_tools() { - local declared fn name - declared=$(jq -r '.tools[]?.name // empty' "${MCP_TOOLS_LIST_FILE}" 2>/dev/null) || { - log "ERROR" "Cannot read declared tool names from ${MCP_TOOLS_LIST_FILE}" - return 1 - } - - while IFS= read -r fn; do - [[ -n "${fn}" ]] || continue - name="${fn#tool_}" - if ! printf '%s\n' "${declared}" | grep -qxF -- "${name}"; then - unset -f "${fn}" - log "INFO" "Removed from dispatch, not declared here: ${name}" - fi - done < <(declare -F | awk '{print $3}' | grep '^tool_' || true) -} - # Resolve the effective repository to use for an API call. # Uses the provided repo arg first, then falls back to GH_DEFAULT_REPO. # Args: $1 = repo from tool arguments (may be empty) @@ -380,6 +349,92 @@ _gh_validate_path() { fi } +# Partial-download helper state (see _gh_partial_create). Each tool call runs +# in its own subshell, so these never leak between calls. +_GH_DL_TMP="" +_GH_DL_TRAP_INSTALLED="" + +####################################### +# EXIT handler installed by _gh_partial_create. Removes the in-flight partial +# file, if any. +# +# No earlier EXIT handler is captured or chained here. Each tool call runs in +# a subshell of the server (`( … ) &` in the vendored protocol layer), and a +# subshell that has not set an EXIT trap of its own reports its parent's +# handler to `trap -p` — here, the server's own teardown trap. A captured +# handler can't be told apart from that inherited one, so running it on +# cancellation would run the server's teardown inside the tool's shell. A +# tool's shell has no EXIT handler of its own to preserve today. +# Globals: +# _GH_DL_TMP +####################################### +_gh_partial_cleanup() { + [[ -n "${_GH_DL_TMP}" ]] && rm -f -- "${_GH_DL_TMP}" +} + +####################################### +# Create a download's `.partial.*` sibling and, on first use in this +# shell, install the EXIT trap that removes it on cancellation. +# +# The file is created with `noclobber` in this shell rather than via +# `mktemp`, whose command substitution creates the file in a subshell well +# before this shell can name it for the trap. What remains is the single +# assignment after creation: a SIGTERM there leaves an empty partial file. +# Globals: +# _GH_DL_TMP (set to the created path on success), _GH_DL_TRAP_INSTALLED +# Arguments: +# $1 destination path the partial file sits beside. +# Outputs: +# An error naming the directory on failure. +# Returns: +# 0 on success, 1 after 5 failed attempts or a non-collision failure. +####################################### +_gh_partial_create() { + local dest="$1" + + if [[ -z "${_GH_DL_TRAP_INSTALLED}" ]]; then + trap '_gh_partial_cleanup' EXIT + _GH_DL_TRAP_INSTALLED=1 + fi + + local had_noclobber=1 + [[ -o noclobber ]] || had_noclobber=0 + set -o noclobber + + # The name is recorded only after noclobber has created the file, so the + # trap never removes a file another process left under the same name. + local attempt=0 candidate + _GH_DL_TMP="" + while (( attempt < 5 )); do + attempt=$(( attempt + 1 )) + candidate="${dest}.partial.${BASHPID}.${RANDOM}${RANDOM}" + if { : > "${candidate}"; } 2>/dev/null; then + _GH_DL_TMP="${candidate}" + break + fi + # Not a name collision — e.g. the parent directory is missing. + [[ -e "${candidate}" ]] || break + done + + [[ ${had_noclobber} -eq 1 ]] || set +o noclobber + + if [[ -z "${_GH_DL_TMP}" ]]; then + echo "Error: cannot create a temporary file in $(dirname "${dest}")" + return 1 + fi +} + +####################################### +# Clear partial-download state after a rename or an explicit removal. The +# EXIT trap installed by _gh_partial_create stays in place — see that +# function and _gh_partial_cleanup for why nothing is restored in its place. +# Globals: +# _GH_DL_TMP +####################################### +_gh_partial_finish() { + _GH_DL_TMP="" +} + # Download a file from GitHub to a local path. # Args: $1=owner, $2=repo, $3=remote_path, $4=local_path, $5=ref (optional) _gh_download_file() { @@ -398,25 +453,37 @@ _gh_download_file() { echo "Error: cannot create directory ${parent_dir}" return 1 } + # Write to a sibling and rename once the body is complete. A cancelled call # has its process group killed mid-write, and a half-written file at the - # target path reads as a complete one. The redirect also captures gh's - # diagnostics, which would otherwise land in the file as its contents. - local tmp_path - tmp_path=$(mktemp "${local_path}.partial.XXXXXX") || { - echo "Error: cannot create a temporary file next to ${local_path}" - return 1 - } - "${cmd[@]}" > "${tmp_path}" 2>&1 || { - rm -f "${tmp_path}" - echo "Error: failed to download ${owner}/${repo}/${remote_path}" + # target path reads as a complete one. + _gh_partial_create "${local_path}" || return 1 + local tmp_path="${_GH_DL_TMP}" + + # stderr is captured apart from the body so gh's own diagnostics never + # become the file's contents; a warning on an otherwise successful + # download is logged instead of discarded. + local dl_err="" dl_exit=0 + dl_err=$({ "${cmd[@]}" > "${tmp_path}"; } 2>&1) || dl_exit=$? + if [[ ${dl_exit} -ne 0 ]]; then + rm -f -- "${tmp_path}" + _gh_partial_finish + if [[ -n "${dl_err}" ]]; then + echo "Error: failed to download ${owner}/${repo}/${remote_path}: ${dl_err}" + else + echo "Error: failed to download ${owner}/${repo}/${remote_path}" + fi return 1 - } + fi + [[ -n "${dl_err}" ]] && log "WARN" "gh reported during download of ${owner}/${repo}/${remote_path}: ${dl_err}" + mv -- "${tmp_path}" "${local_path}" || { - rm -f "${tmp_path}" + rm -f -- "${tmp_path}" + _gh_partial_finish echo "Error: cannot write ${local_path}" return 1 } + _gh_partial_finish } # Resolve owner/repo from multiple sources with priority: @@ -499,6 +566,29 @@ _gh_resolve_owner_repo() { return 1 } +####################################### +# Verify the running server's own tools list is present and well-formed +# before startup completes. The protocol layer reads MCP_TOOLS_LIST_FILE +# lazily, once per tools/list or tools/call request, so nothing else catches +# a missing or corrupt list before the first call — the server would start +# cleanly and then fail every tools/list and tools/call request. +# Globals: +# MCP_TOOLS_LIST_FILE +# Outputs: +# An error naming the file, to stderr. +# Returns: +# 0 when the file holds exactly one JSON object, whose `tools` is an array +# (the protocol layer's own reader requires the single object); 1 otherwise. +####################################### +_gh_require_tools_list() { + if ! jq -e -s 'length == 1 and (.[0] | type) == "object" and (.[0].tools | type) == "array"' \ + -- "${MCP_TOOLS_LIST_FILE}" >/dev/null 2>&1; then + log "ERROR" "Tools list is missing or invalid: ${MCP_TOOLS_LIST_FILE}" + echo "Error: tools list is missing or invalid: ${MCP_TOOLS_LIST_FILE}" >&2 + return 1 + fi +} + # Like _gh_resolve_owner_repo, but returns success with empty globals when no # repo source is provided. Use for tools that have a valid no-repo fallback # (e.g. gh's own git-context resolution for issue/PR subcommands inside a clone). diff --git a/plugins/github-mcp/mcp-server-gh/lib/repo.sh b/plugins/github-mcp/mcp-server-gh/lib/repo.sh index f11faf0..431065c 100644 --- a/plugins/github-mcp/mcp-server-gh/lib/repo.sh +++ b/plugins/github-mcp/mcp-server-gh/lib/repo.sh @@ -136,11 +136,9 @@ tool_repo_file() { # call has its process group killed mid-write, and a half-written file # at the target path reads as a complete one. A failed download now also # leaves any existing file at that path alone. - local __tmp_dl - __tmp_dl=$(mktemp "${download_to}.partial.XXXXXX") || { - echo "Error: cannot create a temporary file next to ${download_to}" - return 1 - } + _gh_partial_create "${download_to}" || return 1 + local __tmp_dl="${_GH_DL_TMP}" + local __exit=0 __dl_err="" if [[ "${suppress_errors}" == "true" ]]; then "${cmd[@]}" > "${__tmp_dl}" 2>/dev/null || __exit=$? @@ -149,17 +147,20 @@ tool_repo_file() { __dl_err=$({ "${cmd[@]}" > "${__tmp_dl}"; } 2>&1) || __exit=$? fi if [[ ${__exit} -ne 0 ]]; then - rm -f "${__tmp_dl}" 2>/dev/null + rm -f -- "${__tmp_dl}" + _gh_partial_finish [[ -n "${fallback}" ]] && { echo "${fallback}"; return 0; } [[ -n "${__dl_err}" ]] && { echo "${__dl_err}"; return ${__exit}; } echo "Error: failed to download ${owner}/${repo}/${path}" return ${__exit} fi mv -- "${__tmp_dl}" "${download_to}" || { - rm -f "${__tmp_dl}" 2>/dev/null + rm -f -- "${__tmp_dl}" + _gh_partial_finish echo "Error: cannot write ${download_to}" return 1 } + _gh_partial_finish echo "Downloaded ${owner}/${repo}/${path} to ${download_to}" return 0 fi diff --git a/plugins/github-mcp/mcp-server-gh/server-read.sh b/plugins/github-mcp/mcp-server-gh/server-read.sh index 9c5ff39..073f527 100755 --- a/plugins/github-mcp/mcp-server-gh/server-read.sh +++ b/plugins/github-mcp/mcp-server-gh/server-read.sh @@ -106,7 +106,7 @@ trap 'log "ERROR" "Unexpected error on line ${LINENO}"' ERR _load_gh_config "${PROJECT_ROOT}" _read_gh_config -_gh_unset_undeclared_tools +_gh_require_tools_list log "INFO" "======================================" log "INFO" "GitHub CLI MCP Server starting" diff --git a/plugins/github-mcp/mcp-server-gh/server-write.sh b/plugins/github-mcp/mcp-server-gh/server-write.sh index 27828c5..98c9644 100755 --- a/plugins/github-mcp/mcp-server-gh/server-write.sh +++ b/plugins/github-mcp/mcp-server-gh/server-write.sh @@ -92,9 +92,9 @@ _check_write_enabled() { if [[ "${enabled}" != "true" ]]; then log "INFO" "Write server disabled (enable_write_server != true)" # A shipped empty list rather than one written at startup. The protocol - # layer owns the process's EXIT trap, so a cleanup trap set here never - # runs, and it answers -32603 for a tools list it cannot read — which a - # startup-written file becomes the moment anything removes it. + # layer answers -32603 for a tools list it cannot read, which a + # startup-written file becomes the moment anything removes it, and any + # cleanup it chains into teardown never runs after SIGKILL. MCP_TOOLS_LIST_FILE="${SCRIPT_DIR}/tools-empty.json" export MCP_TOOLS_LIST_FILE else @@ -120,7 +120,7 @@ trap 'log "ERROR" "Unexpected error on line ${LINENO}"' ERR _load_gh_config "${PROJECT_ROOT}" _read_gh_config _check_write_enabled -_gh_unset_undeclared_tools +_gh_require_tools_list log "INFO" "======================================" log "INFO" "GitHub CLI MCP Server (WRITE) starting" diff --git a/plugins/github-mcp/shared/mcpserver_core.sh b/plugins/github-mcp/shared/mcpserver_core.sh index 16b9282..a63ded7 100755 --- a/plugins/github-mcp/shared/mcpserver_core.sh +++ b/plugins/github-mcp/shared/mcpserver_core.sh @@ -364,20 +364,100 @@ handle_tools_list() { create_response "$id" "$result" } +# Look up a tool's entry in the tools list and print its inputSchema, compact. +# The list is the authority on which tools exist: handle_tools_call answers +# from this lookup before it resolves a function, and validate_tool_arguments +# validates through it, so a name one of them treats as declared is never a +# name the other cannot find. The iteration is `.tools[]?`, which also walks +# an object's values, so `{"tools": {"k": {...}}}` declares the entries it holds. +# Args: $1 = tool name +# Outputs: the schema on status 0, and the caller's rejection message otherwise. +# Returns: 0 for exactly one entry carrying a non-null inputSchema; 2 when no +# entry names the tool; 1 when the list cannot be read or searched, names the +# tool more than once, or its one entry declares no inputSchema. +_mcp_tool_schema() { + local tool_name="$1" + + local tools_config lookup count schema rc + # Each failure below is handled explicitly rather than left to errexit, + # because whether errexit applies here depends on the caller's shape. A + # tools list that cannot be read is a rejection and never a skip: a + # validator that could not read its schemas has not validated anything, and + # reporting success there would wave every declared constraint through, + # which is how the absent-list fallback read. + # The jq failure below is a second such branch. read_json_file's object + # gate catches a file-borne document that is not an object in the branch + # above, but the jq branch stays reachable through a file: an object whose + # `tools` holds a non-object element other than `null` passes the gate, and + # `select` then errors on that element — the trailing `?` guards only the + # iteration. + rc=0 + tools_config=$(read_json_file "$MCP_TOOLS_LIST_FILE" 2>/dev/null) || rc=$? + if [[ $rc -ne 0 ]]; then + printf '%s' "Cannot validate arguments for ${tool_name}: the tool list at ${MCP_TOOLS_LIST_FILE} is missing or does not hold one JSON object." + return 1 + fi + # One line, ` `, so the + # count and the schema come out of a single read of the list. `tojson` + # renders an absent inputSchema and a present null alike as `null`. + rc=0 + lookup=$(printf '%s\n' "$tools_config" | jq -r --arg n "$tool_name" \ + '[.tools[]? | select(.name == $n)] | "\(length) \(.[0].inputSchema | tojson)"' 2>/dev/null) || rc=$? + count="${lookup%% *}" + schema="${lookup#* }" + if [[ $rc -ne 0 || ! "$count" =~ ^[0-9]+$ ]]; then + printf '%s' "Cannot validate arguments for ${tool_name}: the tool list at ${MCP_TOOLS_LIST_FILE} does not hold a usable tools list." + return 1 + fi + if [[ "$count" == "0" ]]; then + printf '%s' "Cannot validate arguments for ${tool_name}: the tool list at ${MCP_TOOLS_LIST_FILE} does not declare it." + return 2 + fi + # Two entries for one name would leave the schema that applies to depend on + # which entry a reader takes first. + if [[ "$count" != "1" ]]; then + printf '%s' "Cannot validate arguments for ${tool_name}: the tool list at ${MCP_TOOLS_LIST_FILE} declares it more than once." + return 1 + fi + if [[ "$schema" == "null" ]]; then + printf '%s' "Cannot validate arguments for ${tool_name}: its entry in ${MCP_TOOLS_LIST_FILE} declares no inputSchema." + return 1 + fi + printf '%s' "$schema" +} + # Validate call arguments against the tool's declared inputSchema, rejecting # arguments that are not a JSON object. The enforced keywords, the union-type # rule, the range-bound rule and the diagnostic precedence order are # README.md §API. +# The schema comes from _mcp_tool_schema, so a tool the tools list does not +# declare exactly once with a non-null inputSchema is rejected rather than +# skipped, and so is a tools list that cannot be read, whether it is missing, +# unparseable, or holds a document that is not a JSON object. +# Args: $1 = tool name, $2 = arguments JSON +# On violation: prints a human-readable message to stdout and returns 1. +validate_tool_arguments() { + local tool_name="$1" + local arguments="$2" + + local schema rc + rc=0 + schema=$(_mcp_tool_schema "$tool_name") || rc=$? + if [[ $rc -ne 0 ]]; then + printf '%s' "$schema" + return 1 + fi + _mcp_validate_against_schema "$schema" "$arguments" "$tool_name" +} + +# Check arguments against one inputSchema; validate_tool_arguments' contract +# applies, minus the tools-list lookup its caller has already made. # A declared `integer` is satisfied by a whole-valued number, decided from the # number as jq renders it and not from its double value alone, so a fractional # literal at or above 2^52 = 4503599627370496 is rejected instead of being read # as whole; a rendering that carries an exponent keeps the double-based verdict, # which admits a fractional value below the smallest subnormal double. -# A tool with no entry in the tools list, or whose entry declares no -# inputSchema, is not validated. A tools list that cannot be read is a -# rejection, whether it is missing, unparseable, or holds a document that is -# not a JSON object, so an unreadable list never becomes a silent skip. A jq -# failure is a rejection and never a skip: +# A jq failure is a rejection and never a skip: # a validator that could not evaluate its input has not validated it, and # reporting success there would wave every constraint through. That branch is # defense-in-depth for a direct call rather than a live remote-input guard — @@ -387,43 +467,18 @@ handle_tools_list() { # category: `null`, `false` and every other JSON scalar are parseable, and # `arguments` may be any JSON value, so a client can send them and they reach # this validator. -# Args: $1 = tool name, $2 = arguments JSON +# Args: $1 = inputSchema JSON, $2 = arguments JSON, $3 = tool name, which only +# the evaluation-failure message names # On violation: prints a human-readable message to stdout and returns 1. -validate_tool_arguments() { - local tool_name="$1" +_mcp_validate_against_schema() { + local schema="$1" local arguments="$2" - - local tools_config schema rc - # errexit is off inside this function — handle_tools_call tests it in a - # conditional — so each failure below is handled explicitly rather than - # left to the call site's shape. A tools list that cannot be read is a - # rejection and never a skip: a validator that could not read its schemas - # has not validated anything, and reporting success there would wave every - # declared constraint through, which is how the absent-list fallback read. - # The jq failure below is a second such branch. read_json_file's object - # gate catches a file-borne document that is not an object in the branch - # above, but the jq branch stays reachable through a file: an object whose - # `tools` holds a non-object element passes the gate, and `select` then - # errors on that element — the trailing `?` guards only the iteration. - rc=0 - tools_config=$(read_json_file "$MCP_TOOLS_LIST_FILE" 2>/dev/null) || rc=$? - if [[ $rc -ne 0 ]]; then - printf '%s' "Cannot validate arguments for ${tool_name}: the tool list at ${MCP_TOOLS_LIST_FILE} is missing or does not hold one JSON object." - return 1 - fi - rc=0 - schema=$(echo "$tools_config" | jq -c --arg n "$tool_name" \ - '(.tools[]? | select(.name == $n) | .inputSchema) // empty' 2>/dev/null) || rc=$? - if [[ $rc -ne 0 ]]; then - printf '%s' "Cannot validate arguments for ${tool_name}: the tool list at ${MCP_TOOLS_LIST_FILE} does not hold a usable tools list." - return 1 - fi - [[ -z "$schema" || "$schema" == "null" ]] && return 0 + local tool_name="$3" # A non-object `arguments` is rejected in the first branch because every # constraint below reads `$args | keys`, which errors on any other type and # would take the whole schema down with it. - local message + local message rc rc=0 message=$(jq -n -r \ --argjson schema "$schema" \ @@ -672,14 +727,40 @@ handle_tools_call() { return fi + # The tools list decides which tools exist, not the shell: a sourced + # `tool_*` function the list does not declare, a `tool__cancel` hook + # included, is not a tool. The lookup also returns the schema the validation + # below runs against, so the list is read once per call. + local schema lookup_rc + lookup_rc=0 + schema=$(_mcp_tool_schema "$tool_name") || lookup_rc=$? + if [[ $lookup_rc -eq 2 ]]; then + create_error_response "$id" -32601 "Tool not found: $tool_name" + return + fi + if [[ $lookup_rc -ne 0 ]]; then + log "ERROR" "Tool $tool_name argument validation failed: $schema" + local lookup_result + lookup_result=$(jq -n -c \ + --arg text "$schema" \ + '{"content": [{"type": "text", "text": $text}], "isError": true}') + create_response "$id" "$lookup_result" + return + fi + + # `declare -F` matches a shell function only. `type` also matched an + # executable, alias or builtin of that name, so a `tool_x` on PATH answered + # for a tool the server never defined. local func_name="tool_${tool_name}" - if ! type "$func_name" &>/dev/null; then + if ! declare -F "$func_name" >/dev/null; then create_error_response "$id" -32601 "Tool not found: $tool_name" return fi - local validation_error - if ! validation_error=$(validate_tool_arguments "$tool_name" "$arguments"); then + local validation_error validation_rc + validation_rc=0 + validation_error=$(_mcp_validate_against_schema "$schema" "$arguments" "$tool_name") || validation_rc=$? + if [[ $validation_rc -ne 0 ]]; then log "ERROR" "Tool $tool_name argument validation failed: $validation_error" local invalid_result invalid_result=$(jq -n -c \ @@ -693,12 +774,42 @@ handle_tools_call() { # cancelled by process group. Job-control notices exist only for interactive # shells, so the `set -m` here adds nothing to stderr. # docs/architecture.md §Tool containment. - local output_file - output_file=$(mktemp "${TMPDIR:-/tmp}/mcp-tool-output.XXXXXX") - # Where the wrapper below records the sentinel's pid; _sentinel_pid_file - # carries the derivation and its reason. - local sentinel_file - sentinel_file="$(_sentinel_pid_file "$output_file")" + # Every file of the call lives in one root that `mktemp -d` creates mode 700 + # and exclusively, so no other user can place anything at a name inside it + # and the files below are written with a plain `>`. + # docs/architecture.md §Tool containment. + local call_root root_rc=0 + call_root=$(mktemp -d "${TMPDIR:-/tmp}/mcp-call.XXXXXX" 2>&1) || root_rc=$? + if [[ $root_rc -ne 0 ]]; then + log "ERROR" "Tool $tool_name not run: cannot create its call directory in ${TMPDIR:-/tmp}: ${call_root}" + local setup_result + setup_result=$(jq -n -c \ + --arg text "Cannot run ${tool_name}: its call directory could not be created." \ + '{"content": [{"type": "text", "text": $text}], "isError": true}') + create_response "$id" "$setup_result" + return + fi + # A relative TMPDIR yields a relative root, and a hook that changes + # directory would then miss its own capture file and hand the tool a + # MCP_CALL_TMPDIR that names nothing from where it runs. + [[ $call_root == /* ]] || call_root="$PWD/$call_root" + # The in-flight record holds the output file's path alone, so every reader + # derives the root as its directory; the names below are that contract. + local output_file="$call_root/output" + local sentinel_file="$call_root/sentinel" + local call_tmpdir="$call_root/tmp" + local mkdir_error mkdir_rc=0 + mkdir_error=$(mkdir -m 700 -- "$call_tmpdir" 2>&1) || mkdir_rc=$? + if [[ $mkdir_rc -ne 0 ]]; then + log "ERROR" "Tool $tool_name not run: cannot create MCP_CALL_TMPDIR ${call_tmpdir}: ${mkdir_error}" + _mcp_remove_call_root "$call_root" + local setup_result + setup_result=$(jq -n -c \ + --arg text "Cannot run ${tool_name}: its call directory could not be created." \ + '{"content": [{"type": "text", "text": $text}], "isError": true}') + create_response "$id" "$setup_result" + return + fi local pid local monitor_was_enabled=0 if [[ -o monitor ]]; then @@ -716,13 +827,15 @@ handle_tools_call() { # wrapper that cannot write it leaves the group's raw liveness as the # teardown's only measure, which is slower but not wrong. # docs/architecture.md §Tool containment. - ( exec {lifeline_rd}<"${_MCP_LIFELINE_DIR}/lifeline"; _lifeline_sentinel "${lifeline_rd}" & sentinel_pid=$!; set +e; printf '%s\n' "$sentinel_pid" > "${sentinel_file}"; _reset_tool_dispatch_state; "$func_name" "$arguments" ) \ + # shellcheck disable=SC2030 # MCP_CALL_TMPDIR is exported for this call's wrapper alone; the dispatch shell must not keep it. + ( exec {lifeline_rd}<"${_MCP_LIFELINE_DIR}/lifeline"; _lifeline_sentinel "${lifeline_rd}" & sentinel_pid=$!; set +e; printf '%s\n' "$sentinel_pid" > "${sentinel_file}"; _reset_tool_dispatch_state; export MCP_CALL_TMPDIR="$call_tmpdir"; _mcp_run_tool "$func_name" "$tool_name" "$arguments" "$call_root" ) \ >"$output_file" 2>&1 "$output_file" 2>&1 "$output_file" 2>&1 _cancel` executable found on PATH. local hook="tool_${tool_name}_cancel" - if ! type "$hook" &>/dev/null; then + if ! declare -F "$hook" >/dev/null; then return 0 fi + local call_root="" + local call_tmpdir="" + if [[ -n "$output_file" ]]; then + if _mcp_call_root_of call_root "$output_file"; then + call_tmpdir="${call_root}/tmp" + else + log "WARN" "refusing to derive a call root from ${output_file}: not a call root; cancel hook ${hook} runs with MCP_CALL_TMPDIR unset" + fi + fi + local hook_pid local waited=0 local hook_status=0 @@ -1033,7 +1197,11 @@ _run_cancel_hook() { monitor_was_enabled=1 fi set -m - ( if [[ -n "${_MCP_LIFELINE_FD:-}" ]]; then exec {_MCP_LIFELINE_FD}>&-; fi; set +e; "$hook" "$arguments" ) >/dev/null 2>&1 &-; fi; set +e; if [[ -n "$call_tmpdir" ]]; then export MCP_CALL_TMPDIR="$call_tmpdir"; else unset MCP_CALL_TMPDIR; fi; "$hook" "$arguments" ) >/dev/null 2>&1 /mcp-call.*`. Anything else — a root a stale or + # corrupted in-flight record named — is refused before `rm` or `find` runs, + # so it cannot turn the removal loose on an arbitrary directory. + if [[ $root != /* || ${root##*/} != mcp-call.* ]]; then + log "WARN" "refusing to remove ${root}: not a call root" + return 0 + fi + + # _teardown_tool_call may have removed it: an absent path is not a failure. + [[ -e $root || -L $root ]] || return 0 + + rm_error=$(rm -rf -- "$root" 2>&1) || rm_rc=$? + if [[ -e $root || -L $root ]]; then + # No `--` end-of-options marker: BusyBox `find` may read it as a path, + # and every call site passes an absolute path, which cannot be an option. + chmod_error=$(find "$root" -type d ! -perm -u+rwx -exec chmod u+rwx {} \; 2>&1) || true + rm_rc=0 + rm_error=$(rm -rf -- "$root" 2>&1) || rm_rc=$? + fi + if [[ $rm_rc -ne 0 || -e $root || -L $root ]]; then + log "WARN" "Cannot remove call directory ${root}: ${rm_error}${chmod_error:+ (restoring permissions: ${chmod_error})}" + fi + return 0 +} + +# Run a tool function behind the consumer's optional mcp_before_tool_call hook, +# inside the call's wrapper subshell. Neither the hook nor the tool is forked +# off: both run in the wrapper's shell, so a global variable the hook assigns, +# a directory it changes to, and an EXIT trap it installs reach the tool. The +# shell options are the exception: those the hook sets are put back before the +# tool runs, and so are functions: what the hook defines reaches the tool but +# not the steps this function runs once the hook has returned. +# README.md §Running a step before every tool. +# Args: $1 = tool function name, $2 = tool name, $3 = the call's arguments JSON, +# $4 = the call root, which holds the hook's capture file and markers +# Outputs: a failed hook's captured output on stdout, which the wrapper has +# pointed at the call's output file; otherwise the tool's own output. +# Returns: the hook's status when it is non-zero, else the tool's status +_mcp_run_tool() { + local func_name="$1" + local tool_name="$2" + local arguments="$3" + local call_root="$4" + + # `declare -F` matches a shell function only, as for the tool itself: an + # executable or alias named mcp_before_tool_call never runs as the hook. + if declare -F mcp_before_tool_call >/dev/null; then + local hook_status=0 + local saved_set saved_shopt + saved_set=$(set +o) + saved_shopt=$(shopt -p) + # The marker outlives the wrapper only when the hook never returned — + # it called `exit` or a signal killed the wrapper — which is what + # handle_tools_call answers for once the wrapper is reaped. A bare + # redirection, so no command name the hook could shadow — `:` included — + # is reached and no PATH lookup happens. + # shellcheck disable=SC2188 # a commandless redirection is the point: any command name here is one the hook may have shadowed or taken off PATH. + >"$call_root/hook-running" + # A brace group, not a subshell: its redirection applies without a fork, + # so what the hook sets survives into the tool call below. The capture + # keeps a successful hook's output out of the tool result. + { mcp_before_tool_call "$tool_name" "$arguments"; } >"$call_root/hook-output" 2>&1 || hook_status=$? + # Every step below runs after the hook returned, so each one bypasses + # what the hook may have changed: a function it defined named `set`, + # `shopt` or `printf` must not take over the SDK's own steps, and a PATH + # it rewrote must not decide whether those steps can run. Nothing below + # reaches for an external command — each file is written with a bare + # redirection and the one command run is a builtin — so a hook that sets + # PATH=/nonexistent leaves no marker behind. The restore re-executes the + # snapshot with every command prefixed `builtin` for the same reason, + # since the snapshot's own `set` and `shopt` calls would otherwise + # resolve to the hook's functions. The tool call at the end is + # deliberately not bypassed: a function the hook defines reaches the + # tool. README.md §Running a step before every tool. + # Restored before anything else runs, so an errexit or nounset the hook + # turned on governs neither the steps below nor the tool. + builtin eval "builtin ${saved_set//$'\n'/$'\n'builtin }" + builtin eval "builtin ${saved_shopt//$'\n'/$'\n'builtin }" + # Written whatever the hook's status, and before the refusal branch: this + # plus `hook-running` is what tells handle_tools_call the hook returned + # rather than died. It is left in place with `hook-output`; the whole + # root goes when the call does. + # shellcheck disable=SC2188 # a commandless redirection is the point: any command name here is one the hook may have shadowed or taken off PATH. + >"$call_root/hook-returned" + if [[ $hook_status -ne 0 ]]; then + # Tells handle_tools_call that the non-zero status is the hook's + # refusal, not the tool's failure. + # shellcheck disable=SC2188 # a commandless redirection is the point: any command name here is one the hook may have shadowed or taken off PATH. + >"$call_root/hook-refused" + builtin printf '%s' "$(<"$call_root/hook-output")" + builtin return "$hook_status" + fi + fi + "$func_name" "$arguments" } # The pid recorded in a call's sentinel file. Empty when no record was written — @@ -1178,12 +1474,15 @@ _teardown_tool_call() { local id="$4" local output_file="$5" - _run_cancel_hook "$tool_name" "$arguments" + _run_cancel_hook "$tool_name" "$arguments" "$output_file" - local sentinel_file - sentinel_file="$(_sentinel_pid_file "$output_file")" - local sentinel_pid - sentinel_pid="$(_sentinel_pid_for "$sentinel_file")" + local call_root="" + local sentinel_pid="" + if _mcp_call_root_of call_root "$output_file"; then + sentinel_pid="$(_sentinel_pid_for "${call_root}/sentinel")" + else + log "WARN" "refusing to derive a call root from ${output_file}: not a call root" + fi local alive_waited=0 if kill -0 -- "-$pid" 2>/dev/null; then @@ -1204,7 +1503,11 @@ _teardown_tool_call() { # which is what a cancellation is expected to look like. wait "$pid" || true _kill_tool_group "$pid" "$tool_name" "$output_file" - rm -f -- "$output_file" "$sentinel_file" + # A path that named no call root has no root to remove, and the check above + # already logged the refusal. + if [[ -n "$call_root" ]]; then + _mcp_remove_call_root "$call_root" + fi log "INFO" "Cancelled tools/call $id ($tool_name)" } @@ -1270,7 +1573,7 @@ process_request() { # Computed once here, because both the version arm below and the id-type # gate read this verdict; a second test would be the same question twice. # - # The integer test is `validate_tool_arguments`' `type_ok` test for a + # The integer test is `_mcp_validate_against_schema`'s `type_ok` test for a # declared `integer`, restated for a whole document: `$id` is already the # id's `tojson` rendering, so the value under test is the document jq reads # rather than a sub-value. The duplication is deliberate — the validator's @@ -1401,10 +1704,16 @@ _server_teardown() { local inflight_pid="" local inflight_tool="" local inflight_output="" + local inflight_root="" local inflight_sentinel_file="" if [[ -n "${_MCP_INFLIGHT_FILE:-}" && -s "${_MCP_INFLIGHT_FILE}" ]]; then read -r inflight_pid inflight_tool inflight_output < "${_MCP_INFLIGHT_FILE}" || true - inflight_sentinel_file="$(_sentinel_pid_file "${inflight_output}")" + fi + # The record is not trusted to name a call: the root, and the sentinel path + # beside it, are derived only from an output path this SDK minted. Every + # read and every removal below is aimed through that one checked root. + if _mcp_call_root_of inflight_root "${inflight_output}"; then + inflight_sentinel_file="${inflight_root}/sentinel" fi # A pgid field of `-` is the tombstone: _kill_tool_group wrote it as it @@ -1415,9 +1724,9 @@ _server_teardown() { # behaviour. if [[ -n "${inflight_pid}" && "${inflight_pid}" != "-" ]] \ && kill -0 -- "-${inflight_pid}" 2>/dev/null; then - _run_cancel_hook "${inflight_tool}" "" - # The sentinel this call's wrapper recorded beside the output file is - # left out of the group's liveness exactly as in _teardown_tool_call: it + _run_cancel_hook "${inflight_tool}" "" "${inflight_output}" + # The sentinel this call's wrapper recorded in the call root is left + # out of the group's liveness exactly as in _teardown_tool_call: it # ignores TERM and waits on the lifeline, so counting it would hold this # teardown for the whole grace on every shutdown. local inflight_sentinel @@ -1436,14 +1745,21 @@ _server_teardown() { log "INFO" "Stopped in-flight tool ${inflight_tool} (group ${inflight_pid}) on shutdown" fi - # The tool's output file is named nowhere but the dispatch subshell that - # created it, so a server going down on a signal has only the record to - # reach it; the sentinel pid file beside it is derived from that same path - # and is removed with it. This removal runs for a tombstone record too, - # which is the point of keeping the path in it. A teardown that found no - # call to stop leaves nothing to remove either way. + # The call root is named nowhere but the dispatch subshell that created it, + # so a server going down on a signal has only the record to reach it: the + # root is the directory of the output file the record names, and it is + # removed with everything in it after the group above has been stopped. + # This removal runs for a tombstone record too, which is the point of + # keeping the path in it. A teardown that found no call to stop leaves + # nothing to remove either way. + # The root removed here is the one checked above, so a record naming a path + # outside a call root leaves that path's parent alone. if [[ -n "${inflight_output}" ]]; then - rm -f -- "${inflight_output}" "${inflight_sentinel_file}" + if [[ -n "${inflight_root}" ]]; then + _mcp_remove_call_root "${inflight_root}" + else + log "WARN" "refusing to remove ${inflight_output%/*}: not a call root" + fi fi # The variables are cleared with the files they name: a later step in this @@ -1478,19 +1794,66 @@ _server_teardown() { _MCP_LIFELINE_DIR="" fi + # The caller's own EXIT handler runs here, after the SDK has released its + # files: the one point both routes into this function reach with the cleanup + # done and the shell still alive. It runs in its own background group under + # the cancel-hook grace, because bash defers a trapped signal until a + # foreground subshell finishes, so a handler that blocked would stall + # shutdown — an overrun is killed and logged. `set +e` and a logged, not + # returned, failure give it the no-errexit shape its contract states. Its + # stdout goes to stderr, and its stdin is `/dev/null` rather than the + # inherited client stream a handler could otherwise consume. The variable is + # cleared after the one run, so no later pass over this function — the + # loop's own teardown, then the EXIT trap — can run the handler a second + # time. docs/architecture.md §Shutdown. + if [[ -n "${_MCP_CHAINED_EXIT_TRAP:-}" ]]; then + local chained_pid="" + local chained_waited=0 + local chained_status=0 + local chained_monitor_was_enabled=0 + if [[ -o monitor ]]; then + chained_monitor_was_enabled=1 + fi + set -m + ( set +e; eval "${_MCP_CHAINED_EXIT_TRAP}" ) >&2 /dev/null \ + && [[ $chained_waited -lt $((_MCP_CANCEL_GRACE_SECONDS * 10)) ]]; do + sleep 0.1 + chained_waited=$((chained_waited + 1)) + done + if kill -0 "$chained_pid" 2>/dev/null; then + kill -KILL -- "-$chained_pid" 2>/dev/null || true + wait "$chained_pid" || true + log "WARN" "Chained caller EXIT trap did not finish within ${_MCP_CANCEL_GRACE_SECONDS}s; killed" + else + wait "$chained_pid" || chained_status=$? + if [[ $chained_status -ne 0 ]]; then + log "WARN" "Chained caller EXIT trap failed with status ${chained_status}" + fi + _kill_tool_group "$chained_pid" "" "" + fi + _MCP_CHAINED_EXIT_TRAP="" + fi + _MCP_TEARDOWN_RUNNING=0 - # The signal a trap recorded while this pass ran is what the shell now dies - # by, and only when this pass was itself driven by a signal trap. - # docs/architecture.md §Shutdown. - if [[ "${_MCP_TEARDOWN_FROM_TRAP:-0}" == "1" && -n "${_MCP_PENDING_SIGNAL:-}" ]]; then + # A signal a trap recorded while this pass ran is what the shell now dies + # by. The re-raise runs at the end of every pass, not only a trap-driven + # one, so a signal recorded during a clean-exit teardown still ends the + # shell by that signal. docs/architecture.md §Shutdown. + if [[ -n "${_MCP_PENDING_SIGNAL:-}" ]]; then local pending_signal pending_signal="${_MCP_PENDING_SIGNAL}" _MCP_PENDING_SIGNAL="" trap - "$pending_signal" kill -s "$pending_signal" "$BASHPID" fi - _MCP_TEARDOWN_FROM_TRAP=0 return 0 } @@ -1511,7 +1874,6 @@ _mcp_teardown_on_signal() { return 0 fi - _MCP_TEARDOWN_FROM_TRAP=1 _server_teardown trap - "$signal" kill -s "$signal" "$BASHPID" @@ -1563,8 +1925,40 @@ run_mcp_server() { # dispatch returns, so the in-flight call finishes first and the teardown # then finds the tool already reaped. README.md §Cancelling and shutting # down. - # These five traps replace any handler the caller installed on the same - # signals, EXIT included. AGENTS.md §Stdout discipline. + # The EXIT handler a caller installed before this call is kept rather than + # dropped: the trap below replaces it, and the teardown runs it afterwards. + # Captured only in the shell the caller's trap was installed in — a caller + # that isolates this call in a subshell keeps its handler in the parent, and + # chaining it from the subshell would run it in two shells. A plain variable + # rather than a `local`, because the EXIT trap reads it after this function + # has returned. docs/architecture.md §Shutdown. + _MCP_CHAINED_EXIT_TRAP="" + if [[ "${BASHPID}" == "$$" ]]; then + # A bash subshell inherits its parent's trap table, and `trap -p` reports + # the parent's handler until the subshell itself changes a trap — so + # this substitution's subshell reports this shell's own handler, and the + # handler does not run there. Should a future bash reset traps in a + # subshell the way POSIX describes, this capture would come back empty + # and a handler would be missed, never fired a second time. + local incumbent_trap_text + incumbent_trap_text=$(trap -p EXIT) + # An empty capture is a shell with no EXIT handler; under `set -o posix` + # an unset trap reports `trap -- - EXIT`, whose `-` is not a handler. + # Both store nothing. + if [[ -n "${incumbent_trap_text}" ]]; then + # The re-input form is `trap -- '' EXIT`, so the handler is + # the third word once that line is parsed as the shell would. + eval "set -- ${incumbent_trap_text}" + if [[ -n "${3-}" && "${3-}" != "-" ]]; then + _MCP_CHAINED_EXIT_TRAP="${3-}" + fi + fi + fi + + # The caller's handler on a signal is replaced here: it expects the shell to + # keep running, and this shell has to die by the signal. The EXIT trap is + # the SDK's own, and the caller's EXIT handler runs from the teardown + # instead. docs/architecture.md §Shutdown. trap '_server_teardown' EXIT trap '_mcp_teardown_on_signal INT' INT trap '_mcp_teardown_on_signal TERM' TERM From 8bd775ebc12621b03718dcca9302bfb392ad0e4d Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Fri, 2 Oct 2026 00:00:05 +0200 Subject: [PATCH 2/2] chore(github-mcp): move the changelog entry to unreleased Version bumps land in a separate release commit, so the entry goes under Unreleased and both plugin manifests stay at 4.2.0. Co-Authored-By: Claude Opus 5.5 (1M context) --- plugins/github-mcp/.claude-plugin/plugin.json | 2 +- plugins/github-mcp/.codex-plugin/plugin.json | 2 +- plugins/github-mcp/CHANGELOG.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/github-mcp/.claude-plugin/plugin.json b/plugins/github-mcp/.claude-plugin/plugin.json index e2cd229..884e34b 100644 --- a/plugins/github-mcp/.claude-plugin/plugin.json +++ b/plugins/github-mcp/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "github-mcp", - "version": "4.2.1", + "version": "4.2.0", "description": "GitHub CLI MCP servers wrapping gh for pull requests, issues, CI runs, jobs, commits, search, labels, projects, and reviews. Read server (always active) with 31 tools and write server (opt-in) with 25 tools. Includes SessionStart hook that injects MCP tool directives and PreToolUse hooks that enforce MCP tool usage. Configuration-optional: works without config when gh is authenticated.", "author": { "name": "Shopware Labs" diff --git a/plugins/github-mcp/.codex-plugin/plugin.json b/plugins/github-mcp/.codex-plugin/plugin.json index 651666b..924031a 100644 --- a/plugins/github-mcp/.codex-plugin/plugin.json +++ b/plugins/github-mcp/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "github-mcp", - "version": "4.2.1", + "version": "4.2.0", "description": "GitHub CLI MCP servers for pull requests, issues, CI, repositories, and other GitHub workflows, with shared lifecycle hooks that steer agents to the MCP tools.", "author": { "name": "Shopware Labs" diff --git a/plugins/github-mcp/CHANGELOG.md b/plugins/github-mcp/CHANGELOG.md index 1ab6de9..6c350cf 100644 --- a/plugins/github-mcp/CHANGELOG.md +++ b/plugins/github-mcp/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [4.2.1] - 2026-10-01 +## [Unreleased] ### Fixed