diff --git a/.mcp-sdk.lock b/.mcp-sdk.lock index bcf3c61..c38bc9a 100644 --- a/.mcp-sdk.lock +++ b/.mcp-sdk.lock @@ -1,2 +1,2 @@ -version=v3.0.0 -sha256=3fc8bc7a58fec1a7e9df1fff64d6f022fd14d9561b1faf82bf4e464e70637e46 +version=v5.0.0 +sha256=b8e55b1f05fe64d165412a5bedc8b5baba764812e13ef0821a536a6c58b7cf07 diff --git a/plugin-tests/github-mcp/tool_dispatch_declaration.bats b/plugin-tests/github-mcp/tool_dispatch_declaration.bats new file mode 100644 index 0000000..529949a --- /dev/null +++ b/plugin-tests/github-mcp/tool_dispatch_declaration.bats @@ -0,0 +1,112 @@ +#!/usr/bin/env bats +# bats file_tags=github-mcp,dispatch,gating +# 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. +bats_require_minimum_version 1.11.0 + +load 'test_helper/common_setup' + +setup() { + PROJECT_DIR="${BATS_TEST_TMPDIR}/project" + mkdir -p "${PROJECT_DIR}" +} + +# Ask a server to run one tool with no arguments. A tool that is not dispatchable +# answers "Tool not found"; one that is reports a missing parameter of its own. +call_tool() { + local server="$1" tool="$2" project_root="$3" + printf '{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"%s","arguments":{}}}\n' "$tool" \ + | env PROJECT_ROOT="$project_root" bash "${GH_SERVER_DIR}/${server}" 2>/dev/null \ + | tail -1 \ + | jq -r '.result.content[0].text // .error.message // "no answer"' +} + +# Every tool_* function the given server sources but does not declare. +undeclared_tools() { + local server="$1" tools_file="$2" lib sourced declared + sourced=$( + grep -o 'lib/[a-z_]*\.sh' "${GH_SERVER_DIR}/${server}" | sort -u \ + | while IFS= read -r lib; do + grep -h -o '^tool_[a-z_0-9]*' "${GH_SERVER_DIR}/${lib}" 2>/dev/null + done \ + | sed 's/^tool_//' | sort -u + ) + declared=$(jq -r '.tools[].name' "${GH_SERVER_DIR}/${tools_file}" | sort -u) + comm -23 <(printf '%s\n' "$sourced") <(printf '%s\n' "$declared") +} + +@test "read server does not dispatch any tool it does not declare" { + local tool found=0 + while read -r tool; do + [[ -n "$tool" ]] || continue + found=$(( found + 1 )) + run call_tool "server-read.sh" "$tool" "${PROJECT_DIR}" + assert_success + assert_output --partial "Tool not found: ${tool}" + done < <(undeclared_tools "server-read.sh" "tools-read.json") + + # The read server sources write tools today; if that ever stops being true + # this test would pass while checking nothing. + [[ "$found" -gt 0 ]] +} + +@test "read server still dispatches a tool it declares" { + run call_tool "server-read.sh" "api_read" "${PROJECT_DIR}" + assert_success + refute_output --partial "Tool not found" + assert_output --partial "endpoint" +} + +@test "write server dispatches nothing while it is disabled" { + printf '%s\n' '{"enable_write_server": false}' > "${PROJECT_DIR}/.mcp-gh-tooling.json" + + local tool + for tool in pr_create issue_create label_add label_list api; do + run call_tool "server-write.sh" "$tool" "${PROJECT_DIR}" + assert_success + assert_output --partial "Tool not found: ${tool}" + done +} + +@test "enabled write server dispatches its own tools but not undeclared ones" { + printf '%s\n' '{"enable_write_server": true}' > "${PROJECT_DIR}/.mcp-gh-tooling.json" + + run call_tool "server-write.sh" "pr_create" "${PROJECT_DIR}" + assert_success + refute_output --partial "Tool not found" + + local tool found=0 + while read -r tool; do + [[ -n "$tool" ]] || continue + found=$(( found + 1 )) + run call_tool "server-write.sh" "$tool" "${PROJECT_DIR}" + assert_success + assert_output --partial "Tool not found: ${tool}" + done < <(undeclared_tools "server-write.sh" "tools-write.json") + + [[ "$found" -gt 0 ]] +} + +@test "disabled write server writes no tools list of its own" { + printf '%s\n' '{"enable_write_server": false}' > "${PROJECT_DIR}/.mcp-gh-tooling.json" + + # Compared before and after rather than asserted absent: the server + # directory is shared, and a run of any earlier revision leaves its own + # startup-written list behind. What matters is that this run writes none. + # The shipped tools-empty.json does not match the pattern. + local before after + before=$(find "${GH_SERVER_DIR}" -maxdepth 1 -name 'tools-empty.*.json' | sort) + + printf '%s\n' '{"jsonrpc":"2.0","id":1,"method":"tools/list"}' \ + | env PROJECT_ROOT="${PROJECT_DIR}" bash "${GH_SERVER_DIR}/server-write.sh" >/dev/null 2>&1 + + after=$(find "${GH_SERVER_DIR}" -maxdepth 1 -name 'tools-empty.*.json' | sort) + + [[ "$before" == "$after" ]] +} diff --git a/plugins/github-mcp/.claude-plugin/plugin.json b/plugins/github-mcp/.claude-plugin/plugin.json index 1a0d485..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.1.0", + "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 a06fd7d..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.1.0", + "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/.gitignore b/plugins/github-mcp/.gitignore deleted file mode 100644 index 8113eb0..0000000 --- a/plugins/github-mcp/.gitignore +++ /dev/null @@ -1,3 +0,0 @@ -# Write-server mktemp artifact: an empty tools list written when the write -# server is disabled (mcp-server-gh/server-write.sh). Runtime-only, never tracked. -mcp-server-gh/tools-empty.*.json diff --git a/plugins/github-mcp/CHANGELOG.md b/plugins/github-mcp/CHANGELOG.md index 6559b23..8e7711f 100644 --- a/plugins/github-mcp/CHANGELOG.md +++ b/plugins/github-mcp/CHANGELOG.md @@ -5,6 +5,24 @@ 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.0] - 2026-09-13 + +### Security + +- A server now runs only the tools its own tools list declares. Tool calls resolve to a shell function by name, and `api.sh`, `label.sh`, and `project.sh` are shared by both servers, so every tool one server sourced for the other was callable on it. That put `label_add`, `label_remove`, `project_item_add`, `project_status_set`, and `api` — which takes any HTTP method — on the read server, which is always active and needs no configuration. `enable_write_server` gated what `tools/list` advertised and nothing else, so those five write tools ran against the authenticated `gh` CLI whether or not the write server was enabled. An undeclared tool also has no schema, and argument validation treats a missing schema as nothing to check, so all of them ran with their arguments unvalidated; combined with the unguarded numeric comparisons in `_gh_post_process`, a crafted `max_lines` reached bash arithmetic evaluation and executed a command inside the server process. Tools not declared by the running server are now removed from dispatch at startup and answer `Tool not found`. **Breaking for callers** who reached a write tool through the read server, or any tool through the write server while it was disabled: both now refuse. Every tool a server declares is unaffected. + +### Fixed + +- `repo_file` and `search_code` with `download_to` write to a sibling file and rename it once the body is complete. Both wrote straight to the destination, so a call interrupted partway left a truncated file that reads as a complete one — reachable for the first time now that a cancelled call has its process group killed mid-write. A failed download also no longer removes an existing file at that path, and `search_code` no longer leaves the GitHub API's error text on disk as the file's contents. +- The write server no longer writes its own empty tools list at startup. The file was created with a `mktemp` template whose placeholder was not at the end of the name, so every server on the machine shared one fixed path, and its cleanup ran from an `EXIT` trap the protocol layer replaces. A shipped `mcp-server-gh/tools-empty.json` takes its place. + +### Changed + +- `shared/mcpserver_core.sh` is vendored from [shopwareLabs/bash-mcp-sdk](https://github.com/shopwareLabs/bash-mcp-sdk) `v5.0.0`, up from `v3.0.0`. +- **The servers now require bash 4.1+ and jq 1.7+, and refuse to start below either.** Both floors were already the protocol layer's documented requirements, and a server below them started anyway: on bash 3.2 it died with `{_MCP_LIFELINE_FD}: not found` before speaking, and on jq 1.6 it ran while silently skipping part of the argument validation its schemas declare. A refusal now names the requirement, the version found, and the install command for the platform. **This stops servers that start today.** macOS ships bash 3.2 as `/bin/bash` and no install replaces it, so a Mac needs a newer bash ahead of `/usr/bin` on the `PATH` the MCP host launches the server with — and a host started from the desktop reads no shell profile, so that is not necessarily the `PATH` your terminal has. +- Cancelling a tool call now stops the work. `notifications/cancelled` was previously ignored, so a cancelled `run_logs` or `search_code` ran its `gh` command to completion; the call's process group is now signalled and the command stops. No configuration or tool change was needed for this. +- A malformed request no longer ends a server. A line carrying more than one JSON document, a document that is not an object, a `tools/call` whose `params` is not an object, and a request whose `id` is neither a string nor an integer are each answered and the server keeps reading. Any one of them previously stopped it with no response, leaving every later request unanswered. + ## [4.1.0] - 2026-09-04 ### Changed diff --git a/plugins/github-mcp/README.md b/plugins/github-mcp/README.md index f5aee30..529dc51 100644 --- a/plugins/github-mcp/README.md +++ b/plugins/github-mcp/README.md @@ -357,8 +357,8 @@ Seven read tools need `fields` before a filter has JSON to run on, and `issue_vi ## Dependencies -- **bash** (4.0+) -- **jq** (JSON processor) +- **bash** (4.1+) — the MCP servers refuse to start below this and say so on stderr. macOS ships 3.2 as `/bin/bash` and no install replaces it, so a Mac needs a newer bash on the `PATH` the host launches the server with. +- **jq** (1.7+) — below this, tool argument validation is skipped, so the servers refuse to start rather than validate less than they claim. - **gh** CLI (GitHub CLI, authenticated) ## License diff --git a/plugins/github-mcp/SETUP.md b/plugins/github-mcp/SETUP.md index ff886ec..09c1945 100644 --- a/plugins/github-mcp/SETUP.md +++ b/plugins/github-mcp/SETUP.md @@ -13,9 +13,14 @@ - **Required by**: The gh-tooling MCP server. The CLI must be authenticated with a GitHub account that has access to the repositories you want to work with. ### jq -- **Check**: `jq --version` +- **Check**: `jq --version` (1.7 or newer) - **Install**: https://jqlang.github.io/jq/download/ -- **Required by**: The gh-tooling MCP server (JSON processing) +- **Required by**: The gh-tooling MCP server (JSON processing). Below 1.7 the server refuses to start: tool argument validation cannot hold every constraint it declares on an older jq, and starting anyway would validate less than the schemas promise. + +### bash +- **Check**: `bash --version` (4.1 or newer) +- **Install**: `brew install bash` on macOS; already satisfied by any current Linux distribution +- **Required by**: The gh-tooling MCP server. macOS ships bash 3.2 as `/bin/bash` and no install replaces it, so installing a newer bash is only half the job — its directory has to come before `/usr/bin` on the `PATH` the MCP host launches the server with. A host started from the desktop reads no shell profile, so that `PATH` may not be the one your terminal has. ## Configuration Files diff --git a/plugins/github-mcp/mcp-server-gh/lib/common.sh b/plugins/github-mcp/mcp-server-gh/lib/common.sh index 10cae7c..e19603f 100644 --- a/plugins/github-mcp/mcp-server-gh/lib/common.sh +++ b/plugins/github-mcp/mcp-server-gh/lib/common.sh @@ -36,6 +36,37 @@ _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) @@ -367,10 +398,25 @@ _gh_download_file() { echo "Error: cannot create directory ${parent_dir}" return 1 } - "${cmd[@]}" > "${local_path}" 2>&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}" return 1 } + mv -- "${tmp_path}" "${local_path}" || { + rm -f "${tmp_path}" + echo "Error: cannot write ${local_path}" + return 1 + } } # Resolve owner/repo from multiple sources with priority: diff --git a/plugins/github-mcp/mcp-server-gh/lib/repo.sh b/plugins/github-mcp/mcp-server-gh/lib/repo.sh index e60f325..f11faf0 100644 --- a/plugins/github-mcp/mcp-server-gh/lib/repo.sh +++ b/plugins/github-mcp/mcp-server-gh/lib/repo.sh @@ -132,20 +132,34 @@ tool_repo_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. 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 + } local __exit=0 __dl_err="" if [[ "${suppress_errors}" == "true" ]]; then - "${cmd[@]}" > "${download_to}" 2>/dev/null || __exit=$? + "${cmd[@]}" > "${__tmp_dl}" 2>/dev/null || __exit=$? else # Capture stderr separately — don't write API errors into the download file - __dl_err=$({ "${cmd[@]}" > "${download_to}"; } 2>&1) || __exit=$? + __dl_err=$({ "${cmd[@]}" > "${__tmp_dl}"; } 2>&1) || __exit=$? fi if [[ ${__exit} -ne 0 ]]; then - rm -f "${download_to}" 2>/dev/null + rm -f "${__tmp_dl}" 2>/dev/null [[ -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 + echo "Error: cannot write ${download_to}" + return 1 + } 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 d1c63fc..9c5ff39 100755 --- a/plugins/github-mcp/mcp-server-gh/server-read.sh +++ b/plugins/github-mcp/mcp-server-gh/server-read.sh @@ -106,6 +106,7 @@ trap 'log "ERROR" "Unexpected error on line ${LINENO}"' ERR _load_gh_config "${PROJECT_ROOT}" _read_gh_config +_gh_unset_undeclared_tools 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 2b7d5a3..27828c5 100755 --- a/plugins/github-mcp/mcp-server-gh/server-write.sh +++ b/plugins/github-mcp/mcp-server-gh/server-write.sh @@ -91,12 +91,12 @@ _check_write_enabled() { fi if [[ "${enabled}" != "true" ]]; then log "INFO" "Write server disabled (enable_write_server != true)" - local empty_tools - empty_tools=$(mktemp "${SCRIPT_DIR}/tools-empty.XXXXXX.json") - printf '{"tools":[]}\n' > "${empty_tools}" - MCP_TOOLS_LIST_FILE="${empty_tools}" + # 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. + MCP_TOOLS_LIST_FILE="${SCRIPT_DIR}/tools-empty.json" export MCP_TOOLS_LIST_FILE - trap 'rm -f "'"${empty_tools}"'"' EXIT else log "INFO" "Write server enabled" fi @@ -120,6 +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 log "INFO" "======================================" log "INFO" "GitHub CLI MCP Server (WRITE) starting" diff --git a/plugins/github-mcp/mcp-server-gh/tools-empty.json b/plugins/github-mcp/mcp-server-gh/tools-empty.json new file mode 100644 index 0000000..bcd83b2 --- /dev/null +++ b/plugins/github-mcp/mcp-server-gh/tools-empty.json @@ -0,0 +1,3 @@ +{ + "tools": [] +} diff --git a/plugins/github-mcp/shared/mcpserver_core.sh b/plugins/github-mcp/shared/mcpserver_core.sh index 9729465..16b9282 100755 --- a/plugins/github-mcp/shared/mcpserver_core.sh +++ b/plugins/github-mcp/shared/mcpserver_core.sh @@ -1,7 +1,212 @@ #!/usr/bin/env bash # MCP Server Core - JSON-RPC 2.0 Protocol Handler # Based on Model Context Protocol specification -# Requires: bash 4+, jq 1.7+ +# Requires: bash 4.1+, jq 1.7+ + +# Pre-flight dependency guards for the two floors the header line declares. +# This block alone uses no construct newer than bash 3.2, sits above the file's +# own `set -euo pipefail`, and diagnoses on stderr. AGENTS.md §Pre-flight +# guards. + +# Print remediation lines for a failed dependency check. `dep` is `bash` or +# `jq`. Output lands on stdout so the function stays pure and testable, which +# makes the `>&2` at every call site mandatory: a call site that forgets it +# writes the hint into the JSON-RPC stream. `uname` is the only external command +# reached for, and an absent or failing `uname` falls through to the generic +# line rather than failing the hint. +_mcp_install_hint() { + local dep="$1" + + local floor="${dep}" + case "${dep}" in + bash) floor="bash 4.1 or newer" ;; + jq) floor="jq 1.7 or newer" ;; + esac + + local platform="" + if command -v uname >/dev/null 2>&1; then + platform=$(uname -s 2>/dev/null) || platform="" + fi + + case "${platform}" in + Darwin) + # Probe for brew, as the Linux arm probes its package managers: a + # Mac with no Homebrew — MacPorts, nix, a managed image — would + # otherwise be handed a command that does not exist. The generic + # line stands in for it, so such a Mac still gets remediation. + if command -v brew >/dev/null 2>&1; then + printf '%s\n' " brew install ${dep}" + else + printf '%s\n' " install ${floor} with your platform's package manager" + fi + if [[ "${dep}" == "bash" ]]; then + # The install on its own changes nothing: macOS keeps 3.2.57 at + # /bin/bash, and both usual launch paths — a + # `#!/usr/bin/env bash` shebang, and a host manifest whose + # command is `bash` — resolve through PATH. A GUI-launched MCP + # host reads no shell profile, so the PATH it hands the server + # is the one that has to be ordered. Printed whether or not brew + # was found: a GUI host hands the server a PATH with no brew on + # it, and that is precisely the case where the fix is a PATH + # fix, so this advice cannot sit inside the brew probe. + printf '%s\n' " then place its directory before /usr/bin in the PATH the MCP host launches this server with" + fi + return 0 + ;; + Linux) + if command -v apt-get >/dev/null 2>&1; then + printf '%s\n' " apt-get install ${dep}" + return 0 + fi + if command -v apk >/dev/null 2>&1; then + printf '%s\n' " apk add ${dep}" + return 0 + fi + if command -v dnf >/dev/null 2>&1; then + printf '%s\n' " dnf install ${dep}" + return 0 + fi + ;; + esac + + printf '%s\n' " install ${floor} with your platform's package manager" + return 0 +} + +# Whether bash . is at or above the 4.1 floor. Takes the version +# as arguments so a suite can drive it from a table. AGENTS.md §Pre-flight +# guards. A non-numeric or empty component reads as below the floor. +_mcp_bash_meets_floor() { + local major="${1:-}" + local minor="${2:-}" + + case "${major}" in ''|*[!0-9]*) return 1 ;; esac + case "${minor}" in ''|*[!0-9]*) return 1 ;; esac + + # Arithmetic comparison, with `10#` forcing base 10: the guards above admit + # any all-digit string, and a leading zero is otherwise read as octal, where + # `08` is not a number and bash writes its own complaint to stderr. + # `(( ))` rather than `[[ -gt ]]`: both evaluate an operand arithmetically + # and agree on every value that reaches them here, but ShellCheck's SC2309 + # flags a built-up operand handed to `[[ ]]`'s numeric operators, and the + # arithmetic form states the intent without needing a directive. + if (( 10#${major} > 4 )); then + return 0 + fi + if (( 10#${major} == 4 && 10#${minor} >= 1 )); then + return 0 + fi + return 1 +} + +# Whether a raw `jq --version` string is at or above the 1.7 floor. +# The comparison is numeric, not lexical: `jq-1.10` is above `jq-1.7` and a +# string comparison gets that backwards. A patch component, and any distro +# suffix — attached to the minor or trailing the patch — is ignored, so +# `jq-1.7.1-Debian-1` and `jq-1.7-Debian-1` are both above the floor. +# Anything that does not parse as `jq-.` is below it, a +# prerelease included: `jq-1.7rc1` is not evidence of released 1.7 behavior, +# and refusing what cannot be read is the direction that fails loudly. +# The `case`-based technique is the one the test images use +# (docker/debian.Dockerfile, docker/alpine.Dockerfile), but this helper admits a +# non-numeric patch or distro suffix they reject: the images gate their own +# build, where a version they cannot fully parse is a reason to fail it, while +# this helper decides whether a consumer's server may run. +_mcp_jq_meets_floor() { + local raw="${1:-}" + + case "${raw}" in + jq-*) ;; + *) return 1 ;; + esac + + local version="${raw#jq-}" + case "${version}" in + *.*) ;; + *) return 1 ;; + esac + + local major="${version%%.*}" + local rest="${version#*.}" + local minor="${rest%%.*}" + # A distro suffix may attach to the minor with no patch component between + # them (`jq-1.8-1`); strip it so it cannot reach the digit guard and refuse a + # version that meets the floor. jq's own prereleases append alphanumerics + # with no hyphen (`jq-1.7rc1`) and stay refused; a hyphenated one would + # survive this strip, and no jq release carries one. Major is left + # unstripped: no real version needs it. + minor="${minor%%-*}" + + case "${major}" in ''|*[!0-9]*) return 1 ;; esac + case "${minor}" in ''|*[!0-9]*) return 1 ;; esac + + # Same shape and same reason as _mcp_bash_meets_floor: a leading zero would + # otherwise be read as octal, and `jq-1.08` would fail arithmetically rather + # than compare. + if (( 10#${major} > 1 )); then + return 0 + fi + if (( 10#${major} == 1 && 10#${minor} >= 7 )); then + return 0 + fi + return 1 +} + +# bash floor. Below 4.1 the file still parses, so there is no early error to go +# on: `run_mcp_server` dies on the `exec {_MCP_LIFELINE_FD}<>` redirection with +# `{_MCP_LIFELINE_FD}: not found`, which an MCP host reports as nothing more +# than a server that failed to start. +if [[ -z "${BASH_VERSINFO[0]:-}" ]] \ + || ! _mcp_bash_meets_floor "${BASH_VERSINFO[0]:-}" "${BASH_VERSINFO[1]:-}"; then + printf '%s\n' "bash-mcp-sdk: bash 4.1 or newer is required; this shell reports ${BASH_VERSION:-unknown}." >&2 + _mcp_install_hint bash >&2 + # `return` when this file is sourced, `exit` when it is executed; the + # complaint `return` raises outside a function is suppressed because it is + # only ever raised on the executed path. + # Known gap: a consumer that sources this file without `set -e` carries on + # past the refusal and reaches `run_mcp_server: command not found`, with the + # message above already on stderr. An unconditional `exit` would close that + # gap and take the BATS runner down with it — the suites source this file + # into the test process. + # shellcheck disable=SC2317 # `return` succeeds only on the sourced path; on the executed path it fails and `exit 1` is what runs. + return 1 2>/dev/null || exit 1 +fi + +# jq presence. +if ! command -v jq >/dev/null 2>&1; then + printf '%s\n' "bash-mcp-sdk: jq was not found on PATH; jq 1.7 or newer is required." >&2 + _mcp_install_hint jq >&2 + # shellcheck disable=SC2317 # `return` succeeds only on the sourced path; on the executed path it fails and `exit 1` is what runs. + return 1 2>/dev/null || exit 1 +fi + +# jq floor. Below 1.7 the server starts and answers every request, and the +# damage is silent: `validate_tool_arguments` reads a number back through +# `tojson` to catch a fraction the conversion to a double rounded away, and +# that depends on jq preserving the number literal — jq 1.7's "use decimal +# number literals to preserve precision". Older jq parses every number to a +# double, so `4503599627370496.5` renders whole and passes as an integer. +# Probed once here, at source time, never on the request path. +_mcp_jq_version=$(jq --version 2>/dev/null) || _mcp_jq_version="" +if ! _mcp_jq_meets_floor "${_mcp_jq_version}"; then + if [[ -z "${_mcp_jq_version}" ]]; then + # The presence check above resolved a path, so jq is installed; it just + # did not answer. Wrong architecture, a missing shared library, a file + # that cannot execute. Naming the resolved path points the operator at + # the binary that is broken, and neither a version nor the + # distro-package warning below applies to a jq that is already on the + # machine. + printf '%s\n' "bash-mcp-sdk: jq was found on PATH but could not be run; jq 1.7 or newer is required." >&2 + printf '%s\n' " the jq at $(command -v jq) did not answer 'jq --version'" >&2 + else + printf '%s\n' "bash-mcp-sdk: jq 1.7 or newer is required; jq reports '${_mcp_jq_version}'." >&2 + printf '%s\n' " a distribution package may itself sit below the floor; a newer jq may have to come from elsewhere" >&2 + fi + _mcp_install_hint jq >&2 + # shellcheck disable=SC2317 # `return` succeeds only on the sourced path; on the executed path it fails and `exit 1` is what runs. + return 1 2>/dev/null || exit 1 +fi +unset _mcp_jq_version set -euo pipefail @@ -9,6 +214,18 @@ set -euo pipefail : "${MCP_TOOLS_LIST_FILE:=tools.json}" : "${MCP_LOG_FILE:=/dev/null}" : "${MCP_EXTRA_LOG_FILE:=}" +: "${MCP_LOG_STDERR:=0}" + +# Seconds a cancelled tool call gets to die after SIGTERM before SIGKILL, and +# a tool__cancel hook gets before it is abandoned. Internal constant: +# the README configuration table lists the MCP_* variables a consumer sets, +# and this is not one of them. +# Guarded so the file can be sourced twice in one shell: a second `readonly` on +# a name the first source already marked fails under `set -e` and takes the +# shell with it. +if [[ -z "${_MCP_CANCEL_GRACE_SECONDS:-}" ]]; then + readonly _MCP_CANCEL_GRACE_SECONDS=2 +fi log() { local level="$1" @@ -17,6 +234,7 @@ log() { line="[$(date '+%Y-%m-%d %H:%M:%S')] [$level] $message" echo "$line" >> "$MCP_LOG_FILE" [[ -n "${MCP_EXTRA_LOG_FILE}" ]] && echo "$line" >> "$MCP_EXTRA_LOG_FILE" + [[ "${MCP_LOG_STDERR}" == "1" ]] && printf '%s\n' "$line" >&2 return 0 } @@ -44,14 +262,30 @@ _configure_extra_log_file() { log "INFO" "Extra log file configured: ${MCP_EXTRA_LOG_FILE}" } +# Read one JSON document from a file and print it compact when it is a JSON +# object; prints nothing and returns 1 otherwise. README.md §API. +# The callers both hand this output to `--argjson` and index it — `.tools` in +# handle_tools_list, `.protocolVersion`, `.serverInfo` and `.capabilities` in +# handle_initialize — so only an object is usable: an index of any other type +# either errors or yields `null` in place of the caller's key, and a `null` or +# `false` document is that accident rather than a configuration. Never a +# fallback: each caller answers the failure instead of treating the +# configuration as empty, so a degraded result is not passed off as a correct +# one. `jq -e` is not used to decide the parse — its exit status comes from the +# truthiness of the output rather than from whether the input parsed, and the +# object test this function needs is made explicitly in the program below. read_json_file() { local file="$1" - if [[ -f "$file" ]]; then - cat "$file" - else + if [[ ! -f "$file" ]]; then log "ERROR" "File not found: $file" - echo "{}" + return 1 + fi + local parsed + if ! parsed=$(jq -cs 'if length == 1 and (.[0] | type) == "object" then .[0] else ("expected exactly one JSON object" | halt_error) end' -- "$file" 2>/dev/null); then + log "ERROR" "Not exactly one parseable JSON object: ${file}" + return 1 fi + printf '%s\n' "$parsed" } create_response() { @@ -68,12 +302,22 @@ create_error_response() { local id="$1" local code="$2" local message="$3" + local data="${4:-}" - jq -n -c \ - --argjson id "$id" \ - --argjson code "$code" \ - --arg message "$message" \ - '{"jsonrpc": "2.0", "id": $id, "error": {"code": $code, "message": $message}}' + if [[ -z "$data" ]]; then + jq -n -c \ + --argjson id "$id" \ + --argjson code "$code" \ + --arg message "$message" \ + '{"jsonrpc": "2.0", "id": $id, "error": {"code": $code, "message": $message}}' + else + jq -n -c \ + --argjson id "$id" \ + --argjson code "$code" \ + --arg message "$message" \ + --argjson data "$data" \ + '{"jsonrpc": "2.0", "id": $id, "error": {"code": $code, "message": $message, "data": $data}}' + fi } handle_initialize() { @@ -83,7 +327,10 @@ handle_initialize() { log "INFO" "Handling initialize request" local config - config=$(read_json_file "$MCP_CONFIG_FILE") + if ! config=$(read_json_file "$MCP_CONFIG_FILE"); then + create_error_response "$id" -32603 "Cannot read server configuration: ${MCP_CONFIG_FILE}" + return + fi local result result=$(jq -n -c \ @@ -103,7 +350,10 @@ handle_tools_list() { log "INFO" "Handling tools/list request" local tools_config - tools_config=$(read_json_file "$MCP_TOOLS_LIST_FILE") + if ! tools_config=$(read_json_file "$MCP_TOOLS_LIST_FILE"); then + create_error_response "$id" -32603 "Cannot read tools list: ${MCP_TOOLS_LIST_FILE}" + return + fi local tools tools=$(echo "$tools_config" | jq -c '.tools // []') @@ -114,41 +364,29 @@ handle_tools_list() { create_response "$id" "$result" } -# Validate call arguments against the tool's declared inputSchema. -# Rejects arguments that are not a JSON object, enforces `required` (every -# listed field must be present), when the schema sets -# `additionalProperties: false` rejects any field not in `properties`, -# enforces a declared `type` (string, integer, number, boolean, array, -# object) on any present field, enforces a declared `pattern` against any -# present string-valued field, enforces declared `minimum`, `maximum`, -# `exclusiveMinimum` and `exclusiveMaximum` bounds against any present -# number-valued field, enforces a declared array `items.type` and -# `items.enum` against every element of a present array-valued field, and -# rejects any present field whose schema declares an `enum` when the supplied -# value is not one of the declared values. A declared `type` — on a property -# or on `items` — is either one name or a list of alternatives, and a value -# satisfies it by matching any member; a list that is empty or carries a -# non-string member is malformed and left unenforced. 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 bound that -# is not a number is malformed the same way, which also leaves the draft-04 -# boolean form `"exclusiveMinimum": true` unenforced. Diagnostics take -# precedence in that order — missing, unknown, type, pattern, range, items, -# enum — so a value that fails more than one constraint is reported with the -# most fundamental defect first (a type mismatch is reported before an -# unrelated enum mismatch). +# 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. +# 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 jq failure is a rejection and never a skip: +# 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 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 — -# process_request gates the whole request through `jq -e '.'`, so arguments -# arriving over the protocol are always parseable JSON. The non-object branch -# is NOT in that category: `null`, `false` and every other JSON scalar are -# parseable, so a client can send them and they reach this validator. +# process_request admits only one JSON document per line and answers anything +# that is not a JSON object before dispatch, so arguments arriving over the +# protocol are always parseable JSON. The non-object branch is NOT in that +# 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 # On violation: prints a human-readable message to stdout and returns 1. validate_tool_arguments() { @@ -157,14 +395,27 @@ validate_tool_arguments() { local tools_config schema rc # errexit is off inside this function — handle_tools_call tests it in a - # conditional — so the unreadable-tools-list fallback must be explicit - # rather than left to the call site's shape. - tools_config=$(read_json_file "$MCP_TOOLS_LIST_FILE" 2>/dev/null) || tools_config='{}' + # 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} is not parseable JSON." + 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 @@ -372,10 +623,38 @@ validate_tool_arguments() { return 0 } +# Clear what a tool body must not inherit from the dispatch that runs it: a +# nested dispatch that kept the server-loop state would stop the server as soon +# as the outer call returned. docs/architecture.md §Tool containment. +# A nested dispatch needing cancellation or deferred replay of its own is out of +# scope; this restores the plain-call shape a direct caller gets. +_reset_tool_dispatch_state() { + _MCP_IN_SERVER_LOOP=0 + unset _MCP_SHUTDOWN_FLAG_FILE _MCP_INFLIGHT_FILE _MCP_PARTIAL_FILE +} + handle_tools_call() { local id="$1" local params="$2" + # A tools/call carries its arguments in a params object, so a params of any + # other type is an Invalid params. This gate runs before `.name` and + # `.arguments` are read, so those extractions only ever see an object and + # none of them can fail. That is what makes the crash fix a property of this + # function rather than of its caller: under `set -o posix` bash inherits + # errexit into the command substitution those extractions run in, and a + # failed one ends the whole server instead of answering the request. It also + # keeps a non-object params' `Cannot index ...` jq diagnostics off the + # process's stderr, unrouted through `log`. And it makes handle_tools_call + # safe as a direct public entry point on its own: a caller passing invalid + # JSON makes the `type` substitution print nothing, which is not "object", + # so the gate fails closed. + if [[ "$(printf '%s\n' "$params" | jq -r 'type')" != "object" ]]; then + log "ERROR" "tools/call params is not a JSON object" + create_error_response "$id" -32602 "Invalid params: expected an object" + return + fi + local tool_name tool_name=$(echo "$params" | jq -r '.name // ""') @@ -410,52 +689,651 @@ handle_tools_call() { return fi - local output - local exit_code=0 - output=$("$func_name" "$arguments" 2>&1) || exit_code=$? + # The tool runs in the background under `set -m` so an in-flight call can be + # 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")" + local pid + local monitor_was_enabled=0 + if [[ -o monitor ]]; then + monitor_was_enabled=1 + fi + set -m + # A lifeline is identified by its directory and FIFO, never by + # _MCP_LIFELINE_FD: process_request closed and cleared that descriptor on + # the way in, so it is empty here on the one path that has a lifeline. + if [[ -n "${_MCP_LIFELINE_DIR:-}" && -p "${_MCP_LIFELINE_DIR}/lifeline" ]]; then + # The wrapper opens its own reader before it starts the tool, so a + # server already gone leaves the wrapper blocked instead of starting an + # orphaned tool. + # The sentinel-pid record is best effort like the in-flight file. A + # 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" ) \ + >"$output_file" 2>&1 "$output_file" 2>&1 "${_MCP_INFLIGHT_FILE}" + fi + + # The poll below reads the client's stdin, and only run_mcp_server's read + # loop owns that stream. A caller that invokes this function directly owns + # its own stdin instead, so it gets the same background child in the same + # process group but a plain blocking wait: reading there would consume + # bytes the caller means to handle, and an EOF on that stream is the + # caller's business rather than a cancellation. + if [[ "${_MCP_IN_SERVER_LOOP:-0}" == "1" ]]; then + _await_tool_call "$id" "$pid" "$tool_name" "$arguments" "$output_file" + else + _wait_tool_call "$pid" "$tool_name" "$output_file" + fi + + # The call's files are released and the in-flight record truncated before + # anything is built from them, so neither the response construction nor the + # deferred replay below can run ahead of the clear. + # The output file and the sentinel pid file go first and the record second: + # those two paths exist nowhere but this subshell, so a teardown landing + # between the steps reads a record naming a path this dispatch has already + # released, and its removal is idempotent, so it loses nothing. + # docs/architecture.md §The in-flight record. + local output="" + if [[ "$_MCP_CANCELLED" -eq 1 ]]; then + # A cancelled call was already answered by its teardown, which sends no + # response for the id, so its output is discarded and only the call's + # files are left to drop here. + rm -f -- "$output_file" "$sentinel_file" + else + output=$(<"$output_file") + rm -f -- "$output_file" "$sentinel_file" + fi + # Cleared once per dispatch, whichever way it ended — completed, cancelled, + # or cleared by the teardown above. Clearing it is what keeps a shutdown from + # signalling a process group that is already gone and whose id could by then + # name something else. + if [[ -n "${_MCP_INFLIGHT_FILE:-}" ]]; then + : > "${_MCP_INFLIGHT_FILE}" + fi + + if [[ "$_MCP_CANCELLED" -eq 0 ]]; then + local exit_code="$_MCP_CHILD_STATUS" + if [[ $exit_code -ne 0 ]]; then + log "ERROR" "Tool $tool_name failed with exit code $exit_code" + local error_result + error_result=$(jq -n -c \ + --arg text "Error executing $tool_name: $output" \ + '{"content": [{"type": "text", "text": $text}], "isError": true}') + create_response "$id" "$error_result" + else + local result + result=$(jq -n -c \ + --arg text "$output" \ + '{"content": [{"type": "text", "text": $text}], "isError": false}') + create_response "$id" "$result" + fi + fi + + # Requests that arrived while the tool ran are answered after the + # in-flight response, in the order they were received. + local deferred deferred_response + if [[ ${#_MCP_DEFERRED_LINES[@]} -gt 0 ]]; then + # A replayed line that is itself a tools/call must not read stdin. This + # subshell is still the one that owns the stream while the replays run, + # so a nested poll would take the bytes of the fragment this dispatch has + # already handed to the read loop — or of the next request — as its own + # deferred line and consume them. With the flag cleared the replay takes + # the no-read path, a plain blocking call, so a replayed call runs to + # completion with no cancellation window: a cancellation naming it + # arrives later in the stream and is dropped as matching nothing. Only + # this subshell's copy is cleared, and it is put back before the + # EOF-drain check below reads the state this dispatch actually ended in. + local outer_in_server_loop + outer_in_server_loop="${_MCP_IN_SERVER_LOOP:-0}" + _MCP_IN_SERVER_LOOP=0 + for deferred in "${_MCP_DEFERRED_LINES[@]}"; do + if [[ -z "$deferred" ]]; then + continue + fi + deferred_response="" + deferred_response=$(process_request "$deferred") + if [[ -n "$deferred_response" ]]; then + printf '%s\n' "$deferred_response" + fi + done + _MCP_IN_SERVER_LOOP="$outer_in_server_loop" + fi + + # A line the client had left half-written when the tool ended has already + # left this dispatch through the partial-line file, written by the poll loop + # before it returned. The server's read loop completes it once this dispatch + # has returned: completing it here would hold the response above back behind + # a line the client might never finish. + + # Stdin reached EOF while the tool ran, so no further request can arrive. + # This dispatch is fully answered by now, so the server stops after it. The + # signal travels through a file rather than a variable because everything + # above ran in the command substitution run_mcp_server dispatches in. + if [[ "${_MCP_EOF_DRAIN:-0}" -eq 1 ]]; then + if [[ -n "${_MCP_SHUTDOWN_FLAG_FILE:-}" ]]; then + : >"$_MCP_SHUTDOWN_FLAG_FILE" + else + log "WARN" "No shutdown flag file set; the server loop cannot be signalled" + fi + fi +} + +# Wait for a tool child when the call is not driven by the server's read loop. +# Nothing is read here, so there is no cancellation window: the dispatch is a +# plain blocking call, as it is for a consumer that invokes handle_tools_call +# or process_request itself. The child already leads its own process group, so +# the shape matches the in-loop one apart from the wait. +# Args: $1 = child pid, $2 = tool name, $3 = tool output file +# Sets: _MCP_CANCELLED, _MCP_CHILD_STATUS, _MCP_DEFERRED_LINES, _MCP_EOF_DRAIN +_wait_tool_call() { + local pid="$1" + local tool_name="$2" + local output_file="$3" + + _MCP_CANCELLED=0 + _MCP_CHILD_STATUS=0 + _MCP_DEFERRED_LINES=() + _MCP_EOF_DRAIN=0 + + wait "$pid" || _MCP_CHILD_STATUS=$? + _kill_tool_group "$pid" "$tool_name" "$output_file" +} + +# Poll the client's stdin while a tool child runs, so an in-flight tools/call +# can be cancelled and so requests that arrive mid-call are answered in order +# once it finishes. Cancellation matching is docs/architecture.md §Cancellation; +# the EOF and fragment rules are docs/architecture.md §Partial-line handoff. +# Args: $1 = in-flight request id (JSON), $2 = child pid, $3 = tool name, +# $4 = original arguments JSON, $5 = tool output file +# Sets: _MCP_CANCELLED (1 when the call was cancelled, else 0), +# _MCP_CHILD_STATUS (the child's exit status), _MCP_DEFERRED_LINES, +# _MCP_EOF_DRAIN (1 when stdin reached EOF, so the server loop stops +# after this dispatch), and _MCP_PARTIAL_FILE (the fragment in hand when +# the loop ended, empty when there is none) +_await_tool_call() { + local id="$1" + local pid="$2" + local tool_name="$3" + local arguments="$4" + local output_file="$5" + + _MCP_CANCELLED=0 + _MCP_CHILD_STATUS=0 + _MCP_DEFERRED_LINES=() + _MCP_EOF_DRAIN=0 + + local buffer="" + local chunk="" + local line="" + local fragment="" + local line_ready=0 + local is_cancel=0 + local rc=0 + local status=0 + + while kill -0 "$pid" 2>/dev/null; do + chunk="" + rc=0 + line_ready=0 + # On a timeout, and on EOF reached after some bytes were read, bash + # still stores what it managed to read into the variable, and those + # bytes are gone from the pipe: they have to be kept and joined to the + # rest of the line, or a line the client wrote in more than one piece + # would lose its first fragment. + # The window is short because it is the latency every completed + # tools/call pays when the tool outruns it: the read is what the wait + # blocks on after the tool is already gone. + IFS= read -r -t 0.05 chunk || rc=$? + if [[ $rc -eq 0 ]]; then + line="${buffer}${chunk}" + buffer="" + line_ready=1 + elif [[ $rc -gt 128 ]]; then + buffer="${buffer}${chunk}" + else + # EOF on stdin: the client is gone and nothing further can be + # read, but the call was accepted and its response is still owed + # to whoever reads the stream. Reading stops here and the normal + # completion below answers it; the shutdown flag is set by + # handle_tools_call once that response and the deferred lines are + # written. The last read carries bytes even at EOF, so the + # fragment is the joined buffer and whatever that read stored. + # A fragment is only dropped when it is not JSON: one that already + # parses is the client's last request, sent with no trailing + # newline that can now ever arrive, and it is answerable — 3.0.0's + # `read || [[ -n "$line" ]]` answered it. It goes through the same + # handling as a terminated line below, and the loop then stops. + # A dropped fragment is unanswerable and its content is whatever + # the client wrote, so only its length is recorded. The buffer is + # cleared in both cases: the fragment has been accounted for, and + # nothing below may treat it as still in hand. + fragment="${buffer}${chunk}" + buffer="" + line="" + if [[ -n "$fragment" ]]; then + # The parse test is `jq empty`, which exits zero on any + # parseable input: `jq -e '.'` set its status from the + # truthiness of the output, so a fragment of `false` or `null` + # — both valid JSON — read as a parse failure and was dropped. + # `jq empty` also exits zero on input holding no documents, the + # empty string and whitespace-only input alike, so the fragment + # reaching this gate need not hold one: the `-n` guard above + # excludes only the empty string, and a whitespace-only + # fragment is handed on and answered -32700 — the same answer + # the same bytes get when a newline follows them. + if printf '%s\n' "$fragment" | jq empty >/dev/null 2>&1; then + line="$fragment" + line_ready=1 + else + log "WARN" "Discarding a partial line of ${#fragment} characters left by EOF during tools/call $id ($tool_name)" + fi + fi + _MCP_EOF_DRAIN=1 + if [[ $line_ready -eq 0 ]]; then + break + fi + fi + + if [[ $line_ready -eq 1 ]]; then + is_cancel=0 + # The id is compared as a JSON value on both sides: `--argjson` + # normalizes the in-flight id, jq compares it against the + # requestId as parsed, so 1 does not match "1" and a malformed + # line simply fails to parse and is queued instead of matching. + # The requestId presence guard precedes the comparison: an absent + # requestId reads as null, and a null in-flight id would then equal + # it and cancel an unrelated call. Matched only when the key is + # there, so the clause fails closed for an id no other message + # reaches. + if printf '%s\n' "$line" | jq -e --argjson id "$id" \ + '.jsonrpc == "2.0" and (has("id") | not) and .method == "notifications/cancelled" and (.params | has("requestId")) and .params.requestId == $id' \ + >/dev/null 2>&1; then + is_cancel=1 + fi + if [[ $is_cancel -eq 1 ]]; then + log "INFO" "Cancelling in-flight tools/call $id ($tool_name)" + _teardown_tool_call "$tool_name" "$pid" "$arguments" "$id" "$output_file" + _MCP_CANCELLED=1 + return 0 + fi + _MCP_DEFERRED_LINES+=("$line") + # The line above was the last one this loop can read; a terminated + # line leaves the drain flag clear and the loop continues. + if [[ $_MCP_EOF_DRAIN -eq 1 ]]; then + break + fi + fi + done + + # A fragment still in hand when the loop ended — the child died before the + # line it began was finished — is written out for run_mcp_server's read loop + # to complete, no trailing newline, so the byte stream the client wrote is + # the byte stream that is joined. Nothing is written for an empty fragment, + # and the EOF above already accounts for the one it discarded. The handoff is + # logged for the call it left, never for the fragment: the fragment's content + # is the client's, so only its length is recorded. + if [[ -n "$buffer" && -n "${_MCP_PARTIAL_FILE:-}" ]]; then + printf '%s' "$buffer" > "${_MCP_PARTIAL_FILE}" + log "INFO" "Handing a partial line of ${#buffer} characters out of tools/call $id ($tool_name) to the read loop" + fi + + wait "$pid" || status=$? + _MCP_CHILD_STATUS="$status" + _kill_tool_group "$pid" "$tool_name" "$output_file" +} + +# Run a tool's optional tool__cancel hook, bounded at +# _MCP_CANCEL_GRACE_SECONDS so a wedged hook cannot wedge its caller. The hook +# is consumer code: it gets the call's arguments as its one argument, and +# neither the client's stdin nor the protocol stream. +# Args: $1 = tool name, $2 = the call's arguments JSON. The teardown that runs +# from a signal handler passes an empty string: it reads the tool's name +# and process group out of the in-flight file, which records no +# arguments. +_run_cancel_hook() { + local tool_name="$1" + local arguments="$2" + + local hook="tool_${tool_name}_cancel" + if ! type "$hook" &>/dev/null; then + return 0 + fi + + local hook_pid + local waited=0 + local hook_status=0 + local monitor_was_enabled=0 + if [[ -o monitor ]]; then + 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 /dev/null \ + && [[ $waited -lt $((_MCP_CANCEL_GRACE_SECONDS * 10)) ]]; do + sleep 0.1 + waited=$((waited + 1)) + done + if kill -0 "$hook_pid" 2>/dev/null; then + kill -KILL -- "-$hook_pid" 2>/dev/null || true + wait "$hook_pid" || true + log "WARN" "Cancel hook ${hook} for ${tool_name} did not finish within ${_MCP_CANCEL_GRACE_SECONDS}s; killed" + else + wait "$hook_pid" || hook_status=$? + if [[ $hook_status -ne 0 ]]; then + log "WARN" "Cancel hook ${hook} for ${tool_name} exited with status ${hook_status}" + fi + # The hook leads a group of its own, which the in-flight record does not + # name, so no tombstone is written for it: the call's group is still + # live here and the record has to keep naming it. + _kill_tool_group "$hook_pid" "" "" + fi + return 0 +} + +# Clear what is left of a tool's process group after its wrapper has been +# reaped: the group is the tool's containment boundary, so nothing the tool +# started outlives the call. docs/architecture.md §Tool containment. +# +# The tombstone is written ahead of the check-and-kill, not after it, because +# this function is the one place the group is deliberately emptied: any other +# order leaves a window where the group is empty and the record still names its +# id. Accepted cost of that order — a teardown racing the same sliver removes +# the call's files without signalling, and the lifeline sentinel remains the +# containment backstop. docs/architecture.md §The in-flight record. +# Args: $1 = the wrapper's pid, which is the group id it leads, +# $2 = the call's tool name, $3 = the call's output file path. The last +# two are empty when the group is not the call's own — the cancel hook's +# group is a group of its own, and the record names the call's — and a +# tombstone is written only when both are non-empty, on the server loop +# only, where a trap can read the record. +_kill_tool_group() { + local pid="$1" + local tool_name="$2" + local output_file="$3" + + if [[ "${_MCP_IN_SERVER_LOOP:-0}" == "1" && -n "${_MCP_INFLIGHT_FILE:-}" \ + && -n "$tool_name" && -n "$output_file" ]]; then + printf '%s %s %s\n' "-" "$tool_name" "$output_file" > "${_MCP_INFLIGHT_FILE}" + fi + + if kill -0 -- "-${pid}" 2>/dev/null; then + kill -KILL -- "-${pid}" 2>/dev/null || true + fi + return 0 +} + +# The file a call's wrapper records its sentinel's pid in: a sibling of the +# call's output file, named from the output file's own name so it belongs to +# that call alone. It is derived rather than remembered because a shutdown +# teardown reaches a running call through the in-flight record, which holds the +# output file's path and nothing else. The name deliberately does not begin +# "mcp-tool-output", so it never reads as a second output file of the call. +# Args: $1 = the call's output file path +_sentinel_pid_file() { + local output_file="$1" + + [[ -z "$output_file" ]] && return 0 + printf '%s/mcp-sentinel.%s' "${output_file%/*}" "${output_file##*/}" +} + +# The pid recorded in a call's sentinel file. Empty when no record was written — +# no lifeline, or the wrapper had not reached it yet — which a grace poll reads +# as "unknown" and falls back to the group's raw liveness. +# Args: $1 = the sentinel pid file path +_sentinel_pid_for() { + local sentinel_file="$1" + local sentinel_pid="" + + if [[ -n "$sentinel_file" && -s "$sentinel_file" ]]; then + IFS= read -r sentinel_pid < "$sentinel_file" || true + fi + printf '%s' "$sentinel_pid" +} + +# Whether a tool's process group still holds a live member other than the +# lifeline sentinel. The sentinel waits on the server's lifeline rather than on +# the tool and ignores TERM, so it outlives a tool that dies on the first TERM; +# counting it would hold every cancellation open for the whole grace period. +# A group whose only live member is the sentinel therefore reads as dead, and +# the caller's grace loop ends. The sentinel is not forgotten: _kill_tool_group +# clears whatever is left of the group once the tool is reaped. +# A group the kernel has already released is dead here too — the scan matches +# nothing — but a caller that must not signal a recycled group id gates on +# `kill -0 -- "-"` for that, not on this. +# The members come from a full `ps` listing filtered here rather than from a +# selecting call, which cannot tell an emptied group from a probe that never +# ran. A `ps` that fails at all is unknown liveness, never an empty group: read +# as empty it would end the caller's grace loop at once and skip the SIGKILL +# after it, leaving a tool that ignores TERM alive. +# docs/architecture.md §Cancellation. +# Args: $1 = group id, $2 = sentinel pid (empty when unknown) +_tool_group_has_live_member() { + local pgid="$1" + local sentinel_pid="$2" + + local listing="" + local ps_rc=0 + # Guarded so a missing binary, which reaches a subshell as a 127 exit rather + # than through errexit, cannot end the caller. + listing="$(ps -A -o pgid=,pid= 2>/dev/null)" || ps_rc=$? + if [[ ${ps_rc} -ne 0 ]]; then + kill -0 -- "-${pgid}" 2>/dev/null return fi - local result - result=$(jq -n -c \ - --arg text "$output" \ - '{"content": [{"type": "text", "text": $text}], "isError": false}') + local entry_pgid member + while read -r entry_pgid member _; do + if [[ "${entry_pgid}" != "${pgid}" || -z "${member}" || "${member}" == "${sentinel_pid}" ]]; then + continue + fi + return 0 + done <<< "${listing}" + return 1 +} - create_response "$id" "$result" +# Stop a cancelled tool call: run its optional cancel hook, terminate the +# child's process group, and reap it. A hook or a child that ignores SIGTERM +# gets _MCP_CANCEL_GRACE_SECONDS before being killed, so a wedged consumer tool +# cannot wedge the server; the grace is measured against the group's live +# members other than the sentinel, so a tool that dies on the first TERM is +# reaped at once rather than two seconds later. +# Args: $1 = tool name, $2 = child pid, $3 = original arguments JSON, +# $4 = in-flight request id (JSON), $5 = tool output file +_teardown_tool_call() { + local tool_name="$1" + local pid="$2" + local arguments="$3" + local id="$4" + local output_file="$5" + + _run_cancel_hook "$tool_name" "$arguments" + + local sentinel_file + sentinel_file="$(_sentinel_pid_file "$output_file")" + local sentinel_pid + sentinel_pid="$(_sentinel_pid_for "$sentinel_file")" + + local alive_waited=0 + if kill -0 -- "-$pid" 2>/dev/null; then + # The child leads its own process group, so the negative pid reaches + # the tool and everything it spawned. + kill -TERM -- "-$pid" 2>/dev/null || true + while _tool_group_has_live_member "$pid" "$sentinel_pid" \ + && [[ $alive_waited -lt $((_MCP_CANCEL_GRACE_SECONDS * 10)) ]]; do + sleep 0.1 + alive_waited=$((alive_waited + 1)) + done + if _tool_group_has_live_member "$pid" "$sentinel_pid"; then + kill -KILL -- "-$pid" 2>/dev/null || true + log "WARN" "Tool ${tool_name} ignored SIGTERM; process group killed" + fi + fi + # Reaped once, after the last signal: the status reflects the signal, + # 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" + log "INFO" "Cancelled tools/call $id ($tool_name)" } process_request() { local request="$1" - if ! echo "$request" | jq -e '.' >/dev/null 2>&1; then + if [[ "${_MCP_IN_SERVER_LOOP:-0}" == "1" && -n "${_MCP_LIFELINE_FD:-}" ]]; then + exec {_MCP_LIFELINE_FD}>&- + _MCP_LIFELINE_FD="" + fi + + # Parse gate: exactly one JSON document, of any type. `jq -e '.'` accepted a + # line holding two documents — it reports only its last output — and the id + # filter below then emitted one line per document, so the joined text + # reached `--argjson`, whose rejection ended the server. It also exits + # non-zero on a whole document that is `null` or `false`, both of which are + # parseable, which is why those two answered -32700 while every other + # scalar reached the version arm and answered -32600. The count is read the + # way read_json_file reads a file's, with `jq -cs` and a length test; a line + # jq cannot parse at all fails the substitution, and any other count is a + # line that is not one document. + local doc_count + if ! doc_count=$(printf '%s\n' "$request" | jq -cs 'length' 2>/dev/null); then log "ERROR" "Invalid JSON received" create_error_response "null" -32700 "Parse error: Invalid JSON" return fi + if [[ "$doc_count" != "1" ]]; then + log "ERROR" "Received a line holding ${doc_count} JSON documents; expected exactly one" + create_error_response "null" -32700 "Parse error: Expected exactly one JSON document per line" + return + fi + + # JSON-RPC 2.0 represents a call as a Request object, so a document of any + # other type is an Invalid Request. This gate runs before `.jsonrpc`, `.id`, + # `.method` and `.params` are read, so those extractions only ever see an + # object and none of them can fail. That is what makes the crash fix a + # property of this function rather than of its caller: process_request no + # longer depends on the dispatch site clearing errexit inside the command + # substitution. It also keeps a non-object line's `Cannot index ...` jq + # diagnostics off the process's stderr, unrouted through `log`. + if [[ "$(printf '%s\n' "$request" | jq -r 'type')" != "object" ]]; then + log "ERROR" "JSON-RPC request is not a JSON object" + create_error_response "null" -32600 "Invalid Request: expected a JSON object" + return + fi - local jsonrpc id method params + local jsonrpc id id_is_valid method params jsonrpc=$(echo "$request" | jq -r '.jsonrpc // ""') - id=$(echo "$request" | jq -c '.id // null') + # The id is read by key presence, not with `//`: that operator maps an + # absent key and a present null to the same output, so a request carrying + # `"id": null` was indistinguishable from a notification. `tojson` keeps + # the JSON type of a present id, so 7 stays an integer and "7" a string, + # and an absent key leaves the empty sentinel the notification gate reads. + id=$(echo "$request" | jq -r 'if has("id") then (.id | tojson) else "" end') method=$(echo "$request" | jq -r '.method // ""') params=$(echo "$request" | jq -c '.params // {}') + # Whether the id is of a type a response can echo back: a JSON string or + # integer. JSON-RPC 2.0 §5 allows a response id to be a String, Number or + # Null, so every other type — and the absent id, whose sentinel is the + # empty string — must be answered with a null id rather than reflected. + # 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 + # 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 + # copy sits inside a large jq program string, and factoring the two + # together is a refactor with its own risk. The comment above that copy + # explains why a bare `floor` comparison is not enough. + id_is_valid=0 + if [[ -n "$id" ]] && printf '%s\n' "$id" | jq -e ' + (type == "string") + or ( + (type == "number") + and (. == (. | floor)) + and ((tojson) as $literal + | if ($literal | test("[eE]")) then true + else ($literal | test("\\.[0-9]*[1-9]") | not) + end) + )' >/dev/null 2>&1; then + id_is_valid=1 + fi + if [[ "$jsonrpc" != "2.0" ]]; then log "ERROR" "Invalid JSON-RPC version: $jsonrpc" - create_error_response "$id" -32600 "Invalid Request: jsonrpc must be 2.0" + # This arm runs ahead of the id-type gate, so it settles the response + # id itself: a string or integer is reflected, and the absent sentinel, + # a null, a boolean, a fractional number, an object or an array all + # answer null, which is the response id JSON-RPC reserves for an id + # that could not be echoed. + local response_id="null" + if [[ "$id_is_valid" == "1" ]]; then + response_id="$id" + fi + create_error_response "$response_id" -32600 "Invalid Request: jsonrpc must be 2.0" + return + fi + + # A message with no id key is a JSON-RPC notification and requires no + # response, so none of the arms below can emit one. The id is empty exactly + # when the key is absent. Between requests nothing is in flight: a + # cancellation that arrives here matches no call and is dropped. + if [[ -z "$id" ]]; then + case "$method" in + "notifications/initialized") + log "INFO" "Client initialized" + ;; + "notifications/cancelled") + log "INFO" "Cancellation received with nothing in flight; dropped" + ;; + *) + log "INFO" "Received notification: $method" + ;; + esac return fi - # JSON-RPC notifications have no id and require no response - if [[ "$id" == "null" ]]; then - log "INFO" "Received notification: $method" + # An id that is neither a string nor an integer is neither a request nor a + # notification: MCP requires a request id to be a string or an integer and + # forbids null, and a notification carries no id at all, so an id of any + # other type — including a fractional number — is answered rather than + # dispatched. The validity test above enforces that rule in full; its + # verdict is read here rather than re-run. + if [[ "$id_is_valid" != "1" ]]; then + log "ERROR" "Request id must be a string or an integer" + create_error_response "null" -32600 "Invalid Request: id must be a string or an integer" return fi @@ -469,9 +1347,6 @@ process_request() { "tools/call") handle_tools_call "$id" "$params" ;; - "notifications/initialized") - log "INFO" "Client initialized" - ;; "ping") create_response "$id" '{}' ;; @@ -482,10 +1357,274 @@ process_request() { esac } +# SIGKILL the tool's process group once the server is gone, however it went. +# This sentinel blocks on a read of the server's lifeline FIFO. +# docs/architecture.md §The lifeline. +# Nothing is ever expected to arrive on that read: a byte would end the wait +# early and kill the group while the server is still alive, and the only writer +# in this design never writes. +_lifeline_sentinel() { + local read_fd="$1" + + trap '' TERM INT HUP + + local pgid="" + local entry_pid entry_pgid + while read -r entry_pid entry_pgid _; do + if [[ "${entry_pid}" == "${BASHPID}" ]]; then + pgid="${entry_pgid}" + break + fi + done < <(ps -A -o pid=,pgid= 2>/dev/null) + + local line="" + IFS= read -r -u "${read_fd}" line || true + + kill -KILL -- "-${pgid}" 2>/dev/null || true +} + +# Stop whatever the server still holds, however the server is going down. +# The in-flight group is not this shell's child — it was the dispatch +# subshell's, and that subshell is often already gone — so it is reaped by +# polling the group, never by `wait`. docs/architecture.md §Shutdown. +_server_teardown() { + # A signal that arrives while this is already running would otherwise enter + # it a second time and re-signal a group, and re-run a hook, that the first + # pass has already dealt with. The flag is cleared on the way out rather + # than left set, so a shell that runs a second server still tears that one + # down. + if [[ "${_MCP_TEARDOWN_RUNNING:-0}" == "1" ]]; then + return 0 + fi + _MCP_TEARDOWN_RUNNING=1 + + local inflight_pid="" + local inflight_tool="" + local inflight_output="" + 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 + + # A pgid field of `-` is the tombstone: _kill_tool_group wrote it as it + # emptied the group, so the id it once named is free to belong to some other + # process group by now. Nothing is signalled for it, no cancel hook runs, + # and no grace is polled — only the files the tombstone still names are + # released below. A numeric pgid is a call still running, and keeps today's + # 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 + # ignores TERM and waits on the lifeline, so counting it would hold this + # teardown for the whole grace on every shutdown. + local inflight_sentinel + inflight_sentinel="$(_sentinel_pid_for "${inflight_sentinel_file}")" + kill -TERM -- "-${inflight_pid}" 2>/dev/null || true + local waited=0 + while _tool_group_has_live_member "${inflight_pid}" "${inflight_sentinel}" \ + && [[ $waited -lt $((_MCP_CANCEL_GRACE_SECONDS * 10)) ]]; do + sleep 0.1 + waited=$((waited + 1)) + done + if _tool_group_has_live_member "${inflight_pid}" "${inflight_sentinel}"; then + kill -KILL -- "-${inflight_pid}" 2>/dev/null || true + log "WARN" "Tool ${inflight_tool} ignored SIGTERM; process group killed" + fi + 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. + if [[ -n "${inflight_output}" ]]; then + rm -f -- "${inflight_output}" "${inflight_sentinel_file}" + fi + + # The variables are cleared with the files they name: a later step in this + # shell cannot recreate a path the teardown has released, and a second pass + # over a cleared name skips the removal instead of repeating it against a + # path the shell may since have given to something else. + if [[ -n "${_MCP_INFLIGHT_FILE:-}" ]]; then + rm -f -- "${_MCP_INFLIGHT_FILE}" + _MCP_INFLIGHT_FILE="" + fi + if [[ -n "${_MCP_SHUTDOWN_FLAG_FILE:-}" ]]; then + rm -f -- "${_MCP_SHUTDOWN_FLAG_FILE}" + _MCP_SHUTDOWN_FLAG_FILE="" + fi + # The partial-line handoff is the one of these the server holds for its own + # use rather than a dispatch's, so a teardown that runs at the end of a + # normal loop lifetime removes it here, exactly as the loop's own exit does. + if [[ -n "${_MCP_PARTIAL_FILE:-}" ]]; then + rm -f -- "${_MCP_PARTIAL_FILE}" + _MCP_PARTIAL_FILE="" + fi + # Dropping the last writer is what releases a sentinel still waiting; the + # group already killed above is the normal way out. The variable is cleared + # with the descriptor so a second teardown cannot close a number the shell + # has since given to something else. + if [[ -n "${_MCP_LIFELINE_FD:-}" ]]; then + exec {_MCP_LIFELINE_FD}>&- + _MCP_LIFELINE_FD="" + fi + if [[ -n "${_MCP_LIFELINE_DIR:-}" ]]; then + rm -rf -- "${_MCP_LIFELINE_DIR}" + _MCP_LIFELINE_DIR="" + 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 + 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 +} + +# What every signal trap runs: tear the server down, then die by the signal that +# was sent, with its default disposition restored, so the exit status reports +# death by signal rather than a plain zero. +# A signal that arrives while a teardown is already running cannot re-enter it. +# It is recorded instead, and the pass in progress re-raises it when it ends: +# the teardown finishes its cleanup, and the shell still dies by the signal. +# Re-raising from here instead would abort that pass part-way, skipping the +# SIGKILL to the tool group and the removal of the call's files. +# Args: $1 = the signal name, as the trap spells it +_mcp_teardown_on_signal() { + local signal="$1" + + if [[ "${_MCP_TEARDOWN_RUNNING:-0}" == "1" ]]; then + _MCP_PENDING_SIGNAL="$signal" + return 0 + fi + + _MCP_TEARDOWN_FROM_TRAP=1 + _server_teardown + trap - "$signal" + kill -s "$signal" "$BASHPID" +} + run_mcp_server() { log "INFO" "MCP Server starting..." - while IFS= read -r line || [[ -n "$line" ]]; do + # A cancellation handler runs inside the command substitution below, so a + # shell variable cannot carry its shutdown signal back out. The flag is a + # file instead, created only when the client closes stdin mid-call. + local shutdown_flag + shutdown_flag=$(mktemp "${TMPDIR:-/tmp}/mcp-shutdown.XXXXXX") + rm -f -- "$shutdown_flag" + export _MCP_SHUTDOWN_FLAG_FILE="$shutdown_flag" + + # Where a dispatch records the tool group it started, the tool's name, and + # the file its output is collected in, for the teardown below to find. + # Created once and left in place: its content, not its existence, is the + # signal, and every dispatch clears it on the way out. + local inflight_file + inflight_file=$(mktemp "${TMPDIR:-/tmp}/mcp-inflight.XXXXXX") + export _MCP_INFLIGHT_FILE="$inflight_file" + + # Where a dispatch hands out a line the client had left half-written when + # the call ended. A fragment cannot travel back out of the command + # substitution above as anything but part of its stdout, and prepending it + # there would mean the dispatch had to read the rest of the line first — + # holding back the response the call already earned. The file is created + # empty and never deleted while the server runs: the loop below empties it + # after every read, so its content, not its existence, is the signal. + local partial_file + partial_file=$(mktemp "${TMPDIR:-/tmp}/mcp-partial.XXXXXX") + export _MCP_PARTIAL_FILE="$partial_file" + + # The lifeline: a FIFO the server holds open read-write and never writes to, + # which every tool wrapper's sentinel reads. A writer disappears exactly when + # the last server process holding it dies, whatever killed it — SIGKILL + # included, which no trap can observe. The read-write open never blocks, and + # the descriptor is what makes the server a writer. + local lifeline_dir + lifeline_dir=$(mktemp -d "${TMPDIR:-/tmp}/mcp-lifeline.XXXXXX") + mkfifo "${lifeline_dir}/lifeline" + export _MCP_LIFELINE_DIR="$lifeline_dir" + exec {_MCP_LIFELINE_FD}<>"${lifeline_dir}/lifeline" + + # Known and accepted limitation, at the point it bites: a signal to this + # shell's pid alone while a dispatch runs takes effect only when that + # 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. + trap '_server_teardown' EXIT + trap '_mcp_teardown_on_signal INT' INT + trap '_mcp_teardown_on_signal TERM' TERM + trap '_mcp_teardown_on_signal HUP' HUP + trap '_mcp_teardown_on_signal PIPE' PIPE + + # This loop is what owns the client's stdin, so it is what licenses the + # mid-call poll in _await_tool_call to read that stream. A plain variable, + # not an export: the poll runs in the command substitution below, a + # subshell of this shell, and no other process has any business reading + # the client's side of the protocol. + _MCP_IN_SERVER_LOOP=1 + + # A fragment a dispatch could not finish is prepended to the next line this + # loop reads, so the line is dispatched as the client wrote it. The variable + # lives across iterations because the read that completes the line happens + # after the dispatch that carried the fragment is already gone. + # A fragment may only be joined to a line whose read succeeded: a read that + # fails leaves whatever it stored unterminated, so joining there would form + # a request the client never finished writing and answer a line it left + # open. The one exception is a joined fragment that already parses as JSON + # — a request the client did finish writing and never terminated — and the + # EOF branch below tells that case from an unterminated line by parsing it. + # A line a failed read stored with no fragment in hand is dispatched as + # before — a client that sent its last request without a trailing newline + # is answered rather than ignored. + local partial="" + local read_rc=0 + while true; do + read_rc=0 + IFS= read -r line || read_rc=$? + if [[ $read_rc -ne 0 && -n "$partial" ]]; then + # Nothing further can arrive, so no later read can complete this + # line, and the bytes this read stored are its tail. The joined + # fragment is still a line when it parses as JSON: a client that + # sent its last request with no trailing newline wrote exactly + # that, and 3.0.0's `read || [[ -n "$line" ]]` answered it. It + # falls through to the dispatch below like any other line. + # Otherwise both pieces are dropped together, with only their + # length logged: the content is the client's. + local eof_fragment + eof_fragment="${partial}${line}" + partial="" + # `jq empty`, not `jq -e '.'`: `-e` takes its status from the + # output's truthiness, so a fragment of `false` or `null` read as a + # parse failure. Full reason at the same test in _await_tool_call. + if ! printf '%s\n' "$eof_fragment" | jq empty >/dev/null 2>&1; then + log "WARN" "Discarding a partial line of ${#eof_fragment} characters left by EOF" + break + fi + line="$eof_fragment" + fi + if [[ $read_rc -ne 0 && -z "$line" ]]; then + break + fi + + if [[ -n "$partial" ]]; then + line="${partial}${line}" + partial="" + fi [[ -z "$line" ]] && continue log "INFO" "Received: ${line:0:100}..." @@ -497,7 +1636,27 @@ run_mcp_server() { log "RESPONSE" "${response:0:100}..." echo "$response" fi + + # Read after the response is out, so a dispatch's fragment never holds + # back the response that dispatch owed. Emptying the file hands the next + # fragment a clean slate, and the next dispatch writes one only if it + # ends with a line still unfinished. + if [[ -n "${_MCP_PARTIAL_FILE:-}" && -s "${_MCP_PARTIAL_FILE}" ]]; then + partial="$(<"${_MCP_PARTIAL_FILE}")" + : > "${_MCP_PARTIAL_FILE}" + fi + + if [[ -e "$_MCP_SHUTDOWN_FLAG_FILE" ]]; then + log "INFO" "Client closed stdin during an in-flight call; shutting down" + break + fi done + _MCP_IN_SERVER_LOOP=0 + _server_teardown + rm -f -- "$shutdown_flag" + unset _MCP_SHUTDOWN_FLAG_FILE + rm -f -- "$partial_file" + unset _MCP_PARTIAL_FILE log "INFO" "MCP Server shutting down" } diff --git a/plugins/plugin-setup/skills/github-mcp-setting-up/references/plugin-setup.md b/plugins/plugin-setup/skills/github-mcp-setting-up/references/plugin-setup.md index ff886ec..09c1945 100644 --- a/plugins/plugin-setup/skills/github-mcp-setting-up/references/plugin-setup.md +++ b/plugins/plugin-setup/skills/github-mcp-setting-up/references/plugin-setup.md @@ -13,9 +13,14 @@ - **Required by**: The gh-tooling MCP server. The CLI must be authenticated with a GitHub account that has access to the repositories you want to work with. ### jq -- **Check**: `jq --version` +- **Check**: `jq --version` (1.7 or newer) - **Install**: https://jqlang.github.io/jq/download/ -- **Required by**: The gh-tooling MCP server (JSON processing) +- **Required by**: The gh-tooling MCP server (JSON processing). Below 1.7 the server refuses to start: tool argument validation cannot hold every constraint it declares on an older jq, and starting anyway would validate less than the schemas promise. + +### bash +- **Check**: `bash --version` (4.1 or newer) +- **Install**: `brew install bash` on macOS; already satisfied by any current Linux distribution +- **Required by**: The gh-tooling MCP server. macOS ships bash 3.2 as `/bin/bash` and no install replaces it, so installing a newer bash is only half the job — its directory has to come before `/usr/bin` on the `PATH` the MCP host launches the server with. A host started from the desktop reads no shell profile, so that `PATH` may not be the one your terminal has. ## Configuration Files