refactor(github-mcp)!: vendor the protocol handler from bash-mcp-sdk v1.0.0

[Martin Bens (SpiGAndromeda)]

What

plugins/github-mcp/shared/mcpserver_core.sh is no longer maintained in this repository. It is vendored from shopwareLabs/bash-mcp-sdk at v1.0.0, byte-identical to lib/mcpserver_core.sh at that tag. .mcp-sdk.lock pins the release and its sha256, renovate.json watches it for new ones, and a CI step fails the build when the vendored copy and the lock disagree.

Why now

The file was copied here when the plugin split out of ai-coding-tools, and upstream moved three commits since:

Upstream	What it changed
b2ff1d8	enum enforcement on any present property
d698712	Validation silently skipped for a non-object arguments
f928a47	type, pattern, and array items enforcement

d698712 is the one with teeth, and the bug was live in what we ship. validate_tool_arguments ended its jq pipeline in || true, so an arguments value of null or false made $args | keys error, the failure was swallowed, and the call dispatched with required and additionalProperties unenforced. Those were the only two checks the copy in this repository had; enum arrives with the vendored file.

Breaking change

An argument whose type does not match its schema now returns an isError result instead of reaching gh.

This affects the integer-typed paging and output parameters — limit, max_lines, tail_lines, grep_context_before, grep_context_after, line_start, line_end. A quoted number such as "20" is refused; send a JSON number instead.

Identifier parameters are unaffected: they declare ["integer", "string"] and take both forms. Worth knowing when reading the rest of this PR: v1.0.0 enforces a declared type only when it is a single string, so a union like that one is not checked against either member. Upstream has fixed this on its main branch and has not released it, so the union buys acceptance of both forms rather than validation of them.

Schema fix

Five numeric identifiers were still declared integer-only and would have started refusing the string form clients send: project_view.number, plus issue_number / sub_issue_number on sub_issue_add and sub_issue_remove. They now declare the union like every other identifier.

No tool-function change was needed. All five already read the value through jq -r, which stringifies 123 and "123" identically. sub_issue_add and sub_issue_remove then check the result with _gh_validate_number; project_view checks only that it is non-empty. Only the schemas were narrower than the code.

Boolean options that could not be disabled

pr_comments.paginate and run_logs.failed_only read their value with jq -r '.paginate // true'. jq's // treats a JSON false as absent, so an explicit false came back as true and neither option could be turned off. Sending the string "false" worked around it until the stricter validation above began enforcing the declared boolean type, which left no value a client could send.

Both now use the has() idiom already established for release_list.latest. The // false reads elsewhere are unaffected: their default is false, so // swallowing a false yields the right value anyway.

Tests

Removed plugin-tests/mcp-shared/mcp_argument_validation.bats and plugin-tests/github-mcp/extra_log_file.bats. Both covered functions that belong to the SDK (validate_tool_arguments, handle_tools_call, log, _configure_extra_log_file), which tests them in its own suite.

Added plugin-tests/github-mcp/tool_schemas.bats (9 tests) covering the seam only this repository can check:

• no numeric identifier is integer-only
• every required name exists in properties — otherwise the tool is uncallable under additionalProperties: false
• every declared default satisfies its own type and enum
• identifiers pass the real validator against the shipped registries in both the integer and the string form
• pr_list {"limit": "20"} is refused, naming the parameter

mcp_tool_gh.bats gains a run_logs counterpart to its pr_comments paginate test, and both now send a JSON boolean rather than the string that used to be the only thing that worked.

Vendoring gate

.github/scripts/vendor-mcp-sdk.sh re-vendors at the pinned release; --check compares and exits non-zero on drift, which is what CI runs. .mcp-sdk.lock replaced templates/** in the workflow path filters — that glob has matched nothing since the split, and without the lock in the filters a lock-only change triggers no workflow run at all.

The download is verified against the sha256 in the lock before it is written anywhere, on both the --check and the re-vendor path. Without that, the only thing separating a good payload from a swapped one was a #! on the first line, and a git tag is mutable by whoever owns the repository it points into. curl also carries connect and overall timeouts, since --retry covers a transfer that completed and failed rather than one that stalled.

After Renovate bumps the lock, the refreshed file has to land in the same PR or the check fails — now on the hash as well as the drift comparison. That coupling is deliberate: the alternative is a lock claiming a release the tree does not contain.

Bugs surfaced while exercising the script, fixed here: the EXIT trap read a local of main after it had returned, so under set -u a clean --check printed ok and then exited 1; and chmod 755 -- fails on macOS, where BSD chmod has no end-of-options marker and reads -- as a filename.