fix(github-mcp): vendor bash-mcp-sdk v5.2.0 and drop cancelled partials

.mcp-sdk.lock

@@ -1,2 +1,2 @@
-version=v5.0.0
-sha256=b8e55b1f05fe64d165412a5bedc8b5baba764812e13ef0821a536a6c58b7cf07
+version=v5.2.0
+sha256=b58b62d82c61f96bc279971afa6c86fb84bd2f8635ac3c6c07bf6ddf6faa727f

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 `<dest>.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}" ]
+}

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
 # =============================================================================

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"'
+}

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'

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 `<file>.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_<name>` on `PATH` and a `tool_<name>_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