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/CHANGELOG.md b/plugins/github-mcp/CHANGELOG.md index 8e7711f..6c350cf 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). +## [Unreleased] + +### 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