From b30fbdecd7c100d1f5c8cc4ddd05644ba0cea1c9 Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Thu, 1 Oct 2026 14:53:38 +0200 Subject: [PATCH] fix!: dispatch only tools the tools list declares handle_tools_call decided that a tool existed by checking for a shell function named tool_ with `type`, and validate_tool_arguments returned success for a name it found no tools-list entry for. A sourced function the server never declared was therefore dispatched with its arguments unchecked, and every tool__cancel hook answered as tool _cancel. A consumer that sources shared tool libraries into several servers had to unset the undeclared functions at startup to close this (#29). The new internal _mcp_tool_schema reads the tools list once per call and returns the declared entry's schema. _mcp_validate_against_schema, the validator body moved out of validate_tool_arguments, checks the arguments against that schema. An undeclared name answers -32601 before any function lookup. An unreadable list, a duplicate entry, or an entry with no inputSchema answers isError. Tool and cancel-hook lookups use declare -F, so a PATH executable named tool_x no longer runs. tests/cancellation.bats now declares the five tools its in-process cases dispatch. BREAKING CHANGE: an undeclared tool_ function, including a cancel hook called as a tool, answers -32601 instead of running. A declared entry with no or a null inputSchema answers isError instead of dispatching unvalidated. validate_tool_arguments returns 1 for an undeclared name. Declare every tool a client calls in tools.json with an inputSchema before bumping the pin. A tool function may still call another tool_* function directly in shell. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../software-writer/writing-code.md | 4 +- AGENTS.md | 3 +- CHANGELOG.md | 13 + README.md | 12 +- docs/architecture.md | 19 +- lib/mcpserver_core.sh | 165 +++++++++---- tests/cancellation.bats | 22 +- tests/mcp_argument_validation.bats | 13 +- tests/tool_declaration.bats | 225 ++++++++++++++++++ 9 files changed, 417 insertions(+), 59 deletions(-) create mode 100644 tests/tool_declaration.bats diff --git a/.claude/extensions/software-writer/writing-code.md b/.claude/extensions/software-writer/writing-code.md index 7fdc786..f1200ba 100644 --- a/.claude/extensions/software-writer/writing-code.md +++ b/.claude/extensions/software-writer/writing-code.md @@ -15,9 +15,9 @@ | Check a call's arguments against a schema | ad-hoc `jq` at the call site | `validate_tool_arguments` (`lib/mcpserver_core.sh`) | Enforces the whole `inputSchema` with diagnostics in precedence order missing > unknown > type > pattern > range > items > enum, and treats a jq failure as a rejection, never a skip. | | Drive one request in a test | spawning the stdio loop | `process_request` (`lib/mcpserver_core.sh`) | Parses and routes a single JSON-RPC line, so a suite exercises a server without `run_mcp_server`'s read loop. | - `code.di_pattern` = A consumer's server script is the composition root: it sets and exports the module inputs (`MCP_CONFIG_FILE`, `MCP_TOOLS_LIST_FILE`, `MCP_LOG_FILE`, `MCP_EXTRA_LOG_FILE`, `PROJECT_ROOT`), defines its `tool_` functions, sources `mcpserver_core.sh`, and calls `run_mcp_server`. The module reads its inputs from those exported variables and discovers nothing itself. Inside this repository there is no composition root — the BATS suites build throwaway server scripts in `BATS_TEST_TMPDIR` to play that role. -- `code.export_conventions` = A leading `_` marks an internal function (`_configure_extra_log_file`); every unprefixed function, its argument order, what it writes to stdout, and the consumer-set variables are public API governed by `AGENTS.md` §Compatibility contract. Classify every surface change against that contract before writing it: rename, argument-order, or stdout change is a major bump; a new function, handled method, or enforced schema keyword is a minor bump; tightening the validator is a major bump even though it fixes a hole. A consumer exports a tool by doing both: defining `tool_` and adding a `tools.json` entry with an `inputSchema` — an entry without a schema is dispatched unvalidated. +- `code.export_conventions` = A leading `_` marks an internal function (`_configure_extra_log_file`); every unprefixed function, its argument order, what it writes to stdout, and the consumer-set variables are public API governed by `AGENTS.md` §Compatibility contract. Classify every surface change against that contract before writing it: rename, argument-order, or stdout change is a major bump; a new function, handled method, or enforced schema keyword is a minor bump; tightening the validator is a major bump even though it fixes a hole. A consumer exports a tool by doing both: defining `tool_` and adding a `tools.json` entry with an `inputSchema` — an undeclared function is not dispatched, and an entry without a schema answers an `isError` result. - `code.footgun_additions` = - - **Stdout is the JSON-RPC stream.** Anything written to stdout outside `create_response` / `create_error_response` corrupts the protocol. `validate_tool_arguments` is the one deliberate exception: it prints a human-readable diagnostic and returns 1, which `handle_tools_call` captures into an `isError` result. + - **Stdout is the JSON-RPC stream.** Anything written to stdout outside `create_response` / `create_error_response` corrupts the protocol. `validate_tool_arguments` is the one deliberate exception: it prints a human-readable diagnostic and returns 1 for its caller to capture. `handle_tools_call` does not call it; it builds its `isError` result from the internal `_mcp_tool_schema` and `_mcp_validate_against_schema`, whose stdout every call site captures in a command substitution, except the `_mcp_validate_against_schema` call inside `validate_tool_arguments`, which passes it through as that function's own output. - A tool function is dispatched in a background wrapper subshell that redirects to the call's output file: `( set +e; _reset_tool_dispatch_state; "$func_name" "$arguments" ) >"$output_file" 2>&1 &2`, which keeps them off the protocol stream, and a new call site has to add that redirect. `validate_tool_arguments` is the one deliberate exception — it prints a human-readable message and returns 1, which `handle_tools_call` turns into an `isError` result. `run_mcp_server` also takes over the process's EXIT trap and expects to be that process's last call. The handler it displaces is not lost: `_server_teardown` runs it after the SDK's own cleanup, with its stdout redirected to stderr so it stays off the protocol stream. The consumer contract is stated in `README.md` §Cancelling and shutting down. +Stdout carries the JSON-RPC stream. `run_mcp_server` captures each dispatch's stdout and echoes it, so only response construction writes there: `create_response`, `create_error_response`, and the deferred responses `handle_tools_call` replays for requests that arrived mid-call. Diagnostics go to `log`. `read_json_file` prints the parsed document, and every call site captures it in a command substitution, so that output never reaches the protocol stream. `_mcp_tool_schema` and `_mcp_validate_against_schema` print a schema or a diagnostic to stdout the same way, and every call site captures that in a command substitution except the one inside `validate_tool_arguments`. `_mcp_install_hint` prints its remediation lines to stdout so the function stays pure and directly testable; every call site redirects them to stderr with `>&2`, which keeps them off the protocol stream, and a new call site has to add that redirect. `validate_tool_arguments` is the one deliberate exception: it prints a human-readable message and returns 1, and `handle_tools_call` turns a message of that shape, built by the `_mcp_validate_against_schema` call it makes itself, into an `isError` result. `run_mcp_server` also takes over the process's EXIT trap and expects to be that process's last call. The handler it displaces is not lost: `_server_teardown` runs it after the SDK's own cleanup, with its stdout redirected to stderr so it stays off the protocol stream. The consumer contract is stated in `README.md` §Cancelling and shutting down. ## Pre-flight guards diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c1500f..2f11de4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,19 @@ All notable changes to this project are documented here. The format follows [Kee ## [Unreleased] +### Changed + +- `tools/call` dispatches only tools the tools list declares. A `tool_` function the list does not declare now answers `-32601 Tool not found: `. It was previously dispatched with its arguments unchecked, because the validator skipped a tool it found no entry for. A nested `handle_tools_call` or `process_request` made from inside a tool follows the same rule. +- A `tool__cancel` hook is no longer callable as tool `_cancel` unless the tools list declares `_cancel`. It previously answered as a tool whenever the hook function existed. +- A declared entry with no `inputSchema`, or a `null` one, returns an `isError` result instead of dispatching the tool unvalidated. The message is `Cannot validate arguments for : its entry in declares no inputSchema.` +- A name the tools list declares more than once returns an `isError` result with the message `Cannot validate arguments for : the tool list at declares it more than once.` It previously returned `isError` with `they could not be evaluated against its schema.` +- The tools list is consulted before the `tool_` function. A call for a name with no function now returns the list's `isError` result when the list cannot be read, declares the name twice, or gives it no `inputSchema`. It previously answered `-32601`, which hid the broken list. +- `validate_tool_arguments` rejects a tool the tools list does not declare. It prints `Cannot validate arguments for : the tool list at does not declare it.` and returns 1. It previously returned 0 with no output. + +### Fixed + +- An executable on `PATH` named `tool_` is no longer dispatched as a tool, and one named `tool__cancel` is no longer run as a cancel hook. Both lookups used `type`, which also matches executables. They now match shell functions only. + ## [5.1.0] - 2026-09-15 ### Changed diff --git a/README.md b/README.md index 4aeedfb..f3d31c1 100644 --- a/README.md +++ b/README.md @@ -46,7 +46,7 @@ There is no install step. Copy `lib/mcpserver_core.sh` into your project and `so | `handle_initialize` | The `initialize` handler `process_request` routes to. Answers `protocolVersion`, `serverInfo` and `capabilities`. | | `handle_tools_list` | The `tools/list` handler `process_request` routes to. Answers the `tools` array. | | `handle_tools_call` | The `tools/call` handler `process_request` routes to. A consumer can drive it directly to dispatch one call without the read loop. | -| `validate_tool_arguments` | Check a call's arguments against the tool's `inputSchema`. | +| `validate_tool_arguments` | Check a call's arguments against the tool's `inputSchema`. Rejects a tool that the tools list does not declare exactly once with a non-null `inputSchema`. | | `create_response` | Build a JSON-RPC result envelope. | | `create_error_response` | Build a JSON-RPC error envelope. Optional 4th arg `data` (JSON value) is included when non-empty. | | `log` | Append to `MCP_LOG_FILE`, and to `MCP_EXTRA_LOG_FILE` when set. | @@ -94,9 +94,13 @@ tool_greet() { run_mcp_server ``` +`tools.json` decides which tools exist. A `tools/call` runs `tool_` only when the list declares `` and a shell function of that name is defined. A name the list does not declare answers `-32601`, even when a `tool_` function is sourced. A declared name with no such function answers `-32601` too. An executable, alias or builtin named `tool_` is never dispatched. + +The same rule holds for a nested `handle_tools_call` or `process_request` made from inside a tool. A plain shell call such as `tool_greet "$args"` is not a dispatch, so it runs whether or not the list declares the tool. + A tool may also define an optional `tool__cancel` hook, which the server calls when the call is cancelled. *Cancelling and shutting down* below gives its contract. -The hook is resolved by name. A tool whose own name ends in `_cancel` is therefore also the cancellation hook of whatever precedes that suffix. A tool named `foo_cancel` is dispatched as a tool, and it is called when `foo` is cancelled. Do not name a tool `_cancel` unless that is what you mean. +The hook is resolved by name, and only a shell function counts. A hook is not a tool: `tool_foo_cancel` is callable as tool `foo_cancel` only when `tools.json` declares `foo_cancel`. A declared tool named `foo_cancel` is still the cancellation hook of `foo`, and it is called when `foo` is cancelled. Do not declare a tool `_cancel` unless that is what you mean. Every `inputSchema` in `tools.json` is enforced before the tool function runs. The keywords are `required`, `additionalProperties: false`, `type`, `pattern`, `minimum` / `maximum` / `exclusiveMinimum` / `exclusiveMaximum`, array `items.type` / `items.enum`, and `enum`. @@ -104,7 +108,9 @@ A `type` — on a property or on `items` — may be one name or a list of altern A range bound applies only to a number-valued argument. A string, boolean, or other non-number carries no bound. A bound that is not itself a number is left unenforced, which also covers the JSON Schema draft-04 boolean form `"exclusiveMinimum": true`. -Diagnostics report the most fundamental defect first, in that order. A tool with no `inputSchema` is dispatched unvalidated. +Diagnostics report the most fundamental defect first, in that order. + +A declared tool whose entry has no `inputSchema`, or a `null` one, is not dispatched. The call returns an `isError` result instead. A name `tools.json` declares more than once also returns an `isError` result. A tool that exits non-zero returns its combined output as an `isError` result rather than killing the server. diff --git a/docs/architecture.md b/docs/architecture.md index 95df72b..94a15da 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -23,7 +23,7 @@ The parse gate refuses two different lines. A line jq cannot read fails the subs The id-type gate accepts a JSON string, and a number that is whole in two readings. `floor` converts its input to an IEEE-754 double, and at or above 2^52 the double spacing reaches 1. A literal such as `4503599627370496.5` is therefore already whole as a double, and `floor` alone cannot see its fraction. `tojson` renders the number from the literal jq parsed, which still carries it. -`validate_tool_arguments` holds the same test for a declared `integer`. One gap survives both copies: a rendering that keeps an exponent falls back to the double-based verdict, so `1.5e-400` reads as an integer. +`_mcp_validate_against_schema`, the validator behind `validate_tool_arguments` and `handle_tools_call`, holds the same test for a declared `integer`. One gap survives both copies: a rendering that keeps an exponent falls back to the double-based verdict, so `1.5e-400` reads as an integer. The notification arm sits between the version gate and the id-type gate. An empty id means the key was absent, so the message is a notification and none of the arms emits a response. @@ -33,7 +33,19 @@ A valid id then routes by method. `initialize`, `tools/list` and `tools/call` go ## Tool containment -`handle_tools_call` gates its `params` to an object before it reads `.name` and `.arguments`, so neither extraction can fail on a non-object. A tool name that does not match `^[a-zA-Z_][a-zA-Z0-9_]*$` answers `-32602`. A name with no `tool_` function answers `-32601`. `.arguments` is read with `has("arguments")`, so a present `null` or `false` reaches the validator instead of defaulting to `{}`. +`handle_tools_call` gates its `params` to an object before it reads `.name` and `.arguments`, so neither extraction can fail on a non-object. A tool name that does not match `^[a-zA-Z_][a-zA-Z0-9_]*$` answers `-32602`. `.arguments` is read with `has("arguments")`, so a present `null` or `false` reaches the validator instead of defaulting to `{}`. + +`_mcp_tool_schema` then looks the name up in the tools list with `.tools[]?`. It reads the list once and returns the entry's schema, and the validator checks the arguments against that schema, so the declaration check and the validation never disagree about what the list holds. + +| Lookup outcome | Answer | +|---|---| +| No entry names the tool | `-32601` | +| The list cannot be read, or `select` errors on an element it cannot index, such as a number | `isError` result | +| More than one entry names the tool | `isError` result | +| The one entry has no `inputSchema`, or a `null` one | `isError` result | +| One entry with a schema | continue | + +A declared name with no `tool_` function answers `-32601`. The test is `declare -F`, which matches shell functions only, so an executable on `PATH` or an alias of that name never answers for a tool. `_mcp_validate_against_schema` then checks the arguments against the schema the lookup returned. The tool then runs in the background under `set -m`, which gives the job its own process group. `handle_tools_call` saves the shell's `monitor` setting and restores it after the spawn. The job is a wrapper subshell rather than the tool itself. That lets a second process sit in the tool's group and outlive the tool without outliving the group. @@ -104,7 +116,7 @@ The sentinel is left out of that count. It waits on the lifeline rather than on A `ps` that fails is unknown liveness, not an empty group. The unknown case degrades to `kill -0` on the whole group, which counts the sentinel and costs the full grace. Read as empty it would end the grace loop immediately and skip the SIGKILL, leaving a TERM-immune tool alive. The degraded path delays a kill rather than dropping one. -`_run_cancel_hook` returns at once when the consumer defined no `tool__cancel`. Otherwise it runs the hook in a background group of its own, with stdin `/dev/null` and its output discarded. A hook still alive after the grace is SIGKILLed by group, and the kill is logged. A hook that exits non-zero is logged too, and neither outcome fails the call. +`_run_cancel_hook` returns at once when no shell function `tool__cancel` is defined. It tests with `declare -F`, so an executable of that name on `PATH` is never run as a hook. Otherwise it runs the hook in a background group of its own, with stdin `/dev/null` and its output discarded. A hook still alive after the grace is SIGKILLed by group, and the kill is logged. A hook that exits non-zero is logged too, and neither outcome fails the call. No tombstone is written for the hook's group. The record names the call's group, which is still live at that point and has to stay named. @@ -153,6 +165,7 @@ The branch also sets `_MCP_EOF_DRAIN`. `handle_tools_call` then touches the shut | One JSON-RPC line yields at most one response | `process_request` | §Request lifecycle | | A notification never produces a response | `process_request` | §Request lifecycle | | A request id is echoed back only when it is a string or an integer | `process_request` | §Request lifecycle | +| A `tools/call` reaches only a shell function the tools list declares | `handle_tools_call`, `_mcp_tool_schema` | §Tool containment | | A tool function never reads the client's protocol stream | `handle_tools_call` | §Tool containment | | A tool leaves nothing running once its call ends | `_kill_tool_group` | §Tool containment | | A nested dispatch inside a tool cannot stop the server or overwrite the outer call's record | `_reset_tool_dispatch_state` | §Tool containment | diff --git a/lib/mcpserver_core.sh b/lib/mcpserver_core.sh index dc85619..3637a2b 100755 --- a/lib/mcpserver_core.sh +++ b/lib/mcpserver_core.sh @@ -364,20 +364,100 @@ handle_tools_list() { create_response "$id" "$result" } +# Look up a tool's entry in the tools list and print its inputSchema, compact. +# The list is the authority on which tools exist: handle_tools_call answers +# from this lookup before it resolves a function, and validate_tool_arguments +# validates through it, so a name one of them treats as declared is never a +# name the other cannot find. The iteration is `.tools[]?`, which also walks +# an object's values, so `{"tools": {"k": {...}}}` declares the entries it holds. +# Args: $1 = tool name +# Outputs: the schema on status 0, and the caller's rejection message otherwise. +# Returns: 0 for exactly one entry carrying a non-null inputSchema; 2 when no +# entry names the tool; 1 when the list cannot be read or searched, names the +# tool more than once, or its one entry declares no inputSchema. +_mcp_tool_schema() { + local tool_name="$1" + + local tools_config lookup count schema rc + # Each failure below is handled explicitly rather than left to errexit, + # because whether errexit applies here depends on the caller's shape. A + # tools list that cannot be read is a rejection and never a skip: a + # validator that could not read its schemas has not validated anything, and + # reporting success there would wave every declared constraint through, + # which is how the absent-list fallback read. + # The jq failure below is a second such branch. read_json_file's object + # gate catches a file-borne document that is not an object in the branch + # above, but the jq branch stays reachable through a file: an object whose + # `tools` holds a non-object element other than `null` passes the gate, and + # `select` then errors on that element — the trailing `?` guards only the + # iteration. + rc=0 + tools_config=$(read_json_file "$MCP_TOOLS_LIST_FILE" 2>/dev/null) || rc=$? + if [[ $rc -ne 0 ]]; then + printf '%s' "Cannot validate arguments for ${tool_name}: the tool list at ${MCP_TOOLS_LIST_FILE} is missing or does not hold one JSON object." + return 1 + fi + # One line, ` `, so the + # count and the schema come out of a single read of the list. `tojson` + # renders an absent inputSchema and a present null alike as `null`. + rc=0 + lookup=$(printf '%s\n' "$tools_config" | jq -r --arg n "$tool_name" \ + '[.tools[]? | select(.name == $n)] | "\(length) \(.[0].inputSchema | tojson)"' 2>/dev/null) || rc=$? + count="${lookup%% *}" + schema="${lookup#* }" + if [[ $rc -ne 0 || ! "$count" =~ ^[0-9]+$ ]]; then + printf '%s' "Cannot validate arguments for ${tool_name}: the tool list at ${MCP_TOOLS_LIST_FILE} does not hold a usable tools list." + return 1 + fi + if [[ "$count" == "0" ]]; then + printf '%s' "Cannot validate arguments for ${tool_name}: the tool list at ${MCP_TOOLS_LIST_FILE} does not declare it." + return 2 + fi + # Two entries for one name would leave the schema that applies to depend on + # which entry a reader takes first. + if [[ "$count" != "1" ]]; then + printf '%s' "Cannot validate arguments for ${tool_name}: the tool list at ${MCP_TOOLS_LIST_FILE} declares it more than once." + return 1 + fi + if [[ "$schema" == "null" ]]; then + printf '%s' "Cannot validate arguments for ${tool_name}: its entry in ${MCP_TOOLS_LIST_FILE} declares no inputSchema." + return 1 + fi + printf '%s' "$schema" +} + # Validate call arguments against the tool's declared inputSchema, rejecting # arguments that are not a JSON object. The enforced keywords, the union-type # rule, the range-bound rule and the diagnostic precedence order are # README.md §API. +# The schema comes from _mcp_tool_schema, so a tool the tools list does not +# declare exactly once with a non-null inputSchema is rejected rather than +# skipped, and so is a tools list that cannot be read, whether it is missing, +# unparseable, or holds a document that is not a JSON object. +# Args: $1 = tool name, $2 = arguments JSON +# On violation: prints a human-readable message to stdout and returns 1. +validate_tool_arguments() { + local tool_name="$1" + local arguments="$2" + + local schema rc + rc=0 + schema=$(_mcp_tool_schema "$tool_name") || rc=$? + if [[ $rc -ne 0 ]]; then + printf '%s' "$schema" + return 1 + fi + _mcp_validate_against_schema "$schema" "$arguments" "$tool_name" +} + +# Check arguments against one inputSchema; validate_tool_arguments' contract +# applies, minus the tools-list lookup its caller has already made. # A declared `integer` is satisfied by a whole-valued number, decided from the # number as jq renders it and not from its double value alone, so a fractional # literal at or above 2^52 = 4503599627370496 is rejected instead of being read # as whole; a rendering that carries an exponent keeps the double-based verdict, # which admits a fractional value below the smallest subnormal double. -# A tool with no entry in the tools list, or whose entry declares no -# inputSchema, is not validated. A tools list that cannot be read is a -# rejection, whether it is missing, unparseable, or holds a document that is -# not a JSON object, so an unreadable list never becomes a silent skip. A jq -# failure is a rejection and never a skip: +# A jq failure is a rejection and never a skip: # a validator that could not evaluate its input has not validated it, and # reporting success there would wave every constraint through. That branch is # defense-in-depth for a direct call rather than a live remote-input guard — @@ -387,43 +467,18 @@ handle_tools_list() { # category: `null`, `false` and every other JSON scalar are parseable, and # `arguments` may be any JSON value, so a client can send them and they reach # this validator. -# Args: $1 = tool name, $2 = arguments JSON +# Args: $1 = inputSchema JSON, $2 = arguments JSON, $3 = tool name, which only +# the evaluation-failure message names # On violation: prints a human-readable message to stdout and returns 1. -validate_tool_arguments() { - local tool_name="$1" +_mcp_validate_against_schema() { + local schema="$1" local arguments="$2" - - local tools_config schema rc - # errexit is off inside this function — handle_tools_call tests it in a - # conditional — so each failure below is handled explicitly rather than - # left to the call site's shape. A tools list that cannot be read is a - # rejection and never a skip: a validator that could not read its schemas - # has not validated anything, and reporting success there would wave every - # declared constraint through, which is how the absent-list fallback read. - # The jq failure below is a second such branch. read_json_file's object - # gate catches a file-borne document that is not an object in the branch - # above, but the jq branch stays reachable through a file: an object whose - # `tools` holds a non-object element passes the gate, and `select` then - # errors on that element — the trailing `?` guards only the iteration. - rc=0 - tools_config=$(read_json_file "$MCP_TOOLS_LIST_FILE" 2>/dev/null) || rc=$? - if [[ $rc -ne 0 ]]; then - printf '%s' "Cannot validate arguments for ${tool_name}: the tool list at ${MCP_TOOLS_LIST_FILE} is missing or does not hold one JSON object." - return 1 - fi - rc=0 - schema=$(echo "$tools_config" | jq -c --arg n "$tool_name" \ - '(.tools[]? | select(.name == $n) | .inputSchema) // empty' 2>/dev/null) || rc=$? - if [[ $rc -ne 0 ]]; then - printf '%s' "Cannot validate arguments for ${tool_name}: the tool list at ${MCP_TOOLS_LIST_FILE} does not hold a usable tools list." - return 1 - fi - [[ -z "$schema" || "$schema" == "null" ]] && return 0 + local tool_name="$3" # A non-object `arguments` is rejected in the first branch because every # constraint below reads `$args | keys`, which errors on any other type and # would take the whole schema down with it. - local message + local message rc rc=0 message=$(jq -n -r \ --argjson schema "$schema" \ @@ -672,14 +727,40 @@ handle_tools_call() { return fi + # The tools list decides which tools exist, not the shell: a sourced + # `tool_*` function the list does not declare, a `tool__cancel` hook + # included, is not a tool. The lookup also returns the schema the validation + # below runs against, so the list is read once per call. + local schema lookup_rc + lookup_rc=0 + schema=$(_mcp_tool_schema "$tool_name") || lookup_rc=$? + if [[ $lookup_rc -eq 2 ]]; then + create_error_response "$id" -32601 "Tool not found: $tool_name" + return + fi + if [[ $lookup_rc -ne 0 ]]; then + log "ERROR" "Tool $tool_name argument validation failed: $schema" + local lookup_result + lookup_result=$(jq -n -c \ + --arg text "$schema" \ + '{"content": [{"type": "text", "text": $text}], "isError": true}') + create_response "$id" "$lookup_result" + return + fi + + # `declare -F` matches a shell function only. `type` also matched an + # executable, alias or builtin of that name, so a `tool_x` on PATH answered + # for a tool the server never defined. local func_name="tool_${tool_name}" - if ! type "$func_name" &>/dev/null; then + if ! declare -F "$func_name" >/dev/null; then create_error_response "$id" -32601 "Tool not found: $tool_name" return fi - local validation_error - if ! validation_error=$(validate_tool_arguments "$tool_name" "$arguments"); then + local validation_error validation_rc + validation_rc=0 + validation_error=$(_mcp_validate_against_schema "$schema" "$arguments" "$tool_name") || validation_rc=$? + if [[ $validation_rc -ne 0 ]]; then log "ERROR" "Tool $tool_name argument validation failed: $validation_error" local invalid_result invalid_result=$(jq -n -c \ @@ -1020,8 +1101,10 @@ _run_cancel_hook() { local tool_name="$1" local arguments="$2" + # A function only, as in handle_tools_call: `type` would also run a + # `tool__cancel` executable found on PATH. local hook="tool_${tool_name}_cancel" - if ! type "$hook" &>/dev/null; then + if ! declare -F "$hook" >/dev/null; then return 0 fi @@ -1270,7 +1353,7 @@ process_request() { # Computed once here, because both the version arm below and the id-type # gate read this verdict; a second test would be the same question twice. # - # The integer test is `validate_tool_arguments`' `type_ok` test for a + # The integer test is `_mcp_validate_against_schema`'s `type_ok` test for a # declared `integer`, restated for a whole document: `$id` is already the # id's `tojson` rendering, so the value under test is the document jq reads # rather than a sub-value. The duplication is deliberate — the validator's diff --git a/tests/cancellation.bats b/tests/cancellation.bats index 5fb218a..f95b35c 100644 --- a/tests/cancellation.bats +++ b/tests/cancellation.bats @@ -105,13 +105,23 @@ setup() { export CHILD_PID_FILE="${BATS_TEST_TMPDIR}/child.pid" export CANCEL_HOOK_FILE="${BATS_TEST_TMPDIR}/cancel-hook.json" export HOOK_CHILD_PID_FILE="${BATS_TEST_TMPDIR}/hook-child.pid" - # The direct handle_tools_call tests source the core and call it for tools - # that are not in any list. An unreadable list is a rejection now, so they - # get a readable empty one: unlisted tools are not validated, which is the - # path those tests exercised when no list was set at all. The fixture server - # overrides this with its own list, so only the direct calls see it. + # The direct handle_tools_call tests source the core and dispatch the tools + # they define, so this list declares each of them: the tools list decides + # which tools a tools/call reaches, and a name it does not declare is + # answered -32601 before any function lookup. Every entry carries the + # inputSchema the call is validated against, and an entry without one + # answers an isError result instead. The fixture server overrides this with + # its own list, so only the direct calls see it. export MCP_TOOLS_LIST_FILE="${BATS_TEST_TMPDIR}/tools.json" - printf '{"tools": []}\n' > "${MCP_TOOLS_LIST_FILE}" + printf '%s\n' '{ + "tools": [ + {"name": "null_id_call", "inputSchema": {"type": "object"}}, + {"name": "trivial", "inputSchema": {"type": "object"}}, + {"name": "after_server_loop", "inputSchema": {"type": "object"}}, + {"name": "monitor_state", "inputSchema": {"type": "object"}}, + {"name": "fail_then_continue", "inputSchema": {"type": "object"}} + ] + }' > "${MCP_TOOLS_LIST_FILE}" } teardown() { diff --git a/tests/mcp_argument_validation.bats b/tests/mcp_argument_validation.bats index 71d2a45..ddec4bd 100644 --- a/tests/mcp_argument_validation.bats +++ b/tests/mcp_argument_validation.bats @@ -170,10 +170,17 @@ teardown() { assert_output "" } -@test "validate_tool_arguments: tool absent from the schema list is not validated" { +@test "validate_tool_arguments: a tool the list does not declare is rejected" { run validate_tool_arguments "nonexistent" '{"whatever": 1}' - assert_success - assert_output "" + assert_failure 1 + assert_output "Cannot validate arguments for nonexistent: the tool list at ${MCP_TOOLS_LIST_FILE} does not declare it." +} + +@test "validate_tool_arguments: a declared entry with no inputSchema is rejected" { + printf '%s\n' '{"tools": [{"name": "bare", "description": "no schema"}]}' > "${MCP_TOOLS_LIST_FILE}" + run validate_tool_arguments "bare" '{}' + assert_failure 1 + assert_output "Cannot validate arguments for bare: its entry in ${MCP_TOOLS_LIST_FILE} declares no inputSchema." } @test "validate_tool_arguments: value outside the declared enum fails naming property, value, and allowed values" { diff --git a/tests/tool_declaration.bats b/tests/tool_declaration.bats new file mode 100644 index 0000000..30b8e77 --- /dev/null +++ b/tests/tool_declaration.bats @@ -0,0 +1,225 @@ +#!/usr/bin/env bats +# bats file_tags=mcp-core,tool-declaration +# Pins that the tools list, not the shell, decides which tools a tools/call can +# reach. A tool is dispatched only when the list declares it exactly once with a +# non-null inputSchema and a shell function `tool_` implements it: +# - a sourced `tool_*` function the list does not declare answers -32601, and +# so does a `tool__cancel` hook called as tool `_cancel`; +# - an entry with no inputSchema, a null one, or a name declared twice is an +# isError result rather than an unvalidated dispatch; +# - an executable on PATH answers neither as a tool nor as a cancel hook; +# - an unreadable tools list stays an isError result, never -32601. +# Each "did not run" claim is proved by the marker file the tool would have +# written, not by the response text alone. Requests are driven through +# process_request, the real entry point. +# Before the change every case below except three failed: the dispatched, +# precedence and object-valued-list cases are guards of behavior the change +# keeps, and are labelled as such. +bats_require_minimum_version 1.11.0 + +load "${BATS_TEST_DIRNAME}/test_helper/common_setup" + +setup() { + MCP_LOG_FILE="${BATS_TEST_TMPDIR}/server.log" + MCP_EXTRA_LOG_FILE="" + MCP_CONFIG_FILE="/dev/null" + MCP_TOOLS_LIST_FILE="${BATS_TEST_TMPDIR}/tools.json" + PROJECT_ROOT="${BATS_TEST_TMPDIR}" + export MCP_LOG_FILE MCP_EXTRA_LOG_FILE MCP_CONFIG_FILE MCP_TOOLS_LIST_FILE PROJECT_ROOT + + # shellcheck source=../lib/mcpserver_core.sh + source "${REPO_ROOT}/lib/mcpserver_core.sh" +} + +teardown() { + unset MCP_LOG_FILE MCP_EXTRA_LOG_FILE MCP_CONFIG_FILE MCP_TOOLS_LIST_FILE PROJECT_ROOT +} + +# A tools/call line for tool with , built with jq so the +# name and the arguments reach process_request as the JSON they name. +_call_request() { + local name="$1" + local arguments_json="$2" + jq -nc --arg n "${name}" --argjson a "${arguments_json}" \ + '{jsonrpc: "2.0", id: 1, method: "tools/call", params: {name: $n, arguments: $a}}' +} + +# Write an executable script named into a directory placed first on +# PATH. Run, it creates . +_put_executable_on_path() { + local name="$1" + local marker="$2" + local bin_dir="${BATS_TEST_TMPDIR}/bin" + mkdir -p "${bin_dir}" + printf '#!/usr/bin/env bash\n: > %q\n' "${marker}" > "${bin_dir}/${name}" + chmod +x "${bin_dir}/${name}" + PATH="${bin_dir}:${PATH}" +} + +# Assert is a -32601 `Tool not found: ` error for id 1. +_assert_tool_not_found() { + local response="$1" + local name="$2" + run jq -e --arg m "Tool not found: ${name}" \ + '.id == 1 and .error.code == -32601 and .error.message == $m and (.result == null)' <<< "${response}" + assert_success +} + +# Assert is an isError result for id 1 whose text is exactly . +_assert_is_error_text() { + local response="$1" + local text="$2" + run jq -e --arg t "${text}" \ + '.id == 1 and .result.isError == true and .result.content[0].text == $t and (.error == null)' <<< "${response}" + assert_success +} + +# --- declared and implemented --- + +@test "process_request: a declared, implemented tool is dispatched (guard: unchanged behavior)" { + printf '%s\n' '{"tools": [{"name": "x", "inputSchema": {"type": "object", "properties": {}}}]}' > "${MCP_TOOLS_LIST_FILE}" + local marker="${BATS_TEST_TMPDIR}/x.ran" + tool_x() { : > "${marker}"; printf 'x done\n'; } + + run process_request "$(_call_request x '{}')" + + assert_success + run jq -e '.id == 1 and .result.isError == false and .result.content[0].text == "x done"' <<< "${output}" + assert_success + assert [ -e "${marker}" ] +} + +# --- not declared --- + +@test "process_request: a sourced tool function the list does not declare answers -32601 and does not run" { + printf '%s\n' '{"tools": [{"name": "other", "inputSchema": {"type": "object"}}]}' > "${MCP_TOOLS_LIST_FILE}" + local marker="${BATS_TEST_TMPDIR}/x.ran" + tool_x() { : > "${marker}"; } + + run process_request "$(_call_request x '{}')" + + assert_success + _assert_tool_not_found "${output}" x + assert [ ! -e "${marker}" ] +} + +@test "process_request: a cancel hook is not callable as a tool when only its tool is declared" { + printf '%s\n' '{"tools": [{"name": "foo", "inputSchema": {"type": "object"}}]}' > "${MCP_TOOLS_LIST_FILE}" + local hook_marker="${BATS_TEST_TMPDIR}/foo_cancel.ran" + tool_foo() { printf 'foo done\n'; } + tool_foo_cancel() { : > "${hook_marker}"; } + + run process_request "$(_call_request foo_cancel '{}')" + + assert_success + _assert_tool_not_found "${output}" foo_cancel + assert [ ! -e "${hook_marker}" ] +} + +@test "process_request: an invalid tool name answers -32602 ahead of the declaration lookup (guard: unchanged behavior)" { + printf '%s\n' '{"tools": []}' > "${MCP_TOOLS_LIST_FILE}" + + run process_request "$(_call_request a-b '{}')" + + assert_success + run jq -e '.id == 1 and .error.code == -32602 and .error.message == "Invalid tool name: a-b"' <<< "${output}" + assert_success +} + +# --- declared without a usable schema --- + +@test "process_request: a declared entry with no inputSchema is an isError result and does not run" { + printf '%s\n' '{"tools": [{"name": "x", "description": "no schema"}]}' > "${MCP_TOOLS_LIST_FILE}" + local marker="${BATS_TEST_TMPDIR}/x.ran" + tool_x() { : > "${marker}"; } + + run process_request "$(_call_request x '{}')" + + assert_success + _assert_is_error_text "${output}" "Cannot validate arguments for x: its entry in ${MCP_TOOLS_LIST_FILE} declares no inputSchema." + assert [ ! -e "${marker}" ] +} + +@test "process_request: a declared entry whose inputSchema is null is an isError result and does not run" { + printf '%s\n' '{"tools": [{"name": "x", "inputSchema": null}]}' > "${MCP_TOOLS_LIST_FILE}" + local marker="${BATS_TEST_TMPDIR}/x.ran" + tool_x() { : > "${marker}"; } + + run process_request "$(_call_request x '{}')" + + assert_success + _assert_is_error_text "${output}" "Cannot validate arguments for x: its entry in ${MCP_TOOLS_LIST_FILE} declares no inputSchema." + assert [ ! -e "${marker}" ] +} + +@test "process_request: a tool declared twice is an isError result naming the duplicate and does not run" { + printf '%s\n' '{"tools": [{"name": "x", "inputSchema": {"type": "object"}}, {"name": "x", "inputSchema": {"type": "object", "required": ["n"]}}]}' > "${MCP_TOOLS_LIST_FILE}" + local marker="${BATS_TEST_TMPDIR}/x.ran" + tool_x() { : > "${marker}"; } + + run process_request "$(_call_request x '{}')" + + assert_success + _assert_is_error_text "${output}" "Cannot validate arguments for x: the tool list at ${MCP_TOOLS_LIST_FILE} declares it more than once." + assert [ ! -e "${marker}" ] +} + +@test "process_request: an object-valued tools map declares its entries, and their schema is enforced (guard: unchanged behavior)" { + # `.tools[]?` walks an object's values as well as an array's elements, so + # the declaration check and the validator read this list the same way. + printf '%s\n' '{"tools": {"k": {"name": "x", "inputSchema": {"type": "object", "required": ["n"]}}}}' > "${MCP_TOOLS_LIST_FILE}" + local marker="${BATS_TEST_TMPDIR}/x.ran" + tool_x() { : > "${marker}"; } + + run process_request "$(_call_request x '{}')" + + assert_success + _assert_is_error_text "${output}" "Missing required parameter(s): n." + assert [ ! -e "${marker}" ] +} + +# --- an unreadable tools list --- + +@test "process_request: a missing tools list is an isError result, not -32601, for a tool with no function" { + # No tool_x is defined, so a lookup ordered after the function check would + # answer -32601 and hide the unreadable list. + rm -f -- "${MCP_TOOLS_LIST_FILE}" + + run process_request "$(_call_request x '{}')" + + assert_success + _assert_is_error_text "${output}" "Cannot validate arguments for x: the tool list at ${MCP_TOOLS_LIST_FILE} is missing or does not hold one JSON object." +} + +@test "process_request: a tools list holding a non-object element is an isError result, not -32601, for a tool with no function" { + printf '%s\n' '{"tools": [1]}' > "${MCP_TOOLS_LIST_FILE}" + + run process_request "$(_call_request x '{}')" + + assert_success + _assert_is_error_text "${output}" "Cannot validate arguments for x: the tool list at ${MCP_TOOLS_LIST_FILE} does not hold a usable tools list." +} + +# --- executables on PATH --- + +@test "process_request: a declared tool with no function answers -32601 even when a tool_ executable is on PATH" { + printf '%s\n' '{"tools": [{"name": "x", "inputSchema": {"type": "object"}}]}' > "${MCP_TOOLS_LIST_FILE}" + local marker="${BATS_TEST_TMPDIR}/path-tool.ran" + _put_executable_on_path tool_x "${marker}" + + run process_request "$(_call_request x '{}')" + + assert_success + _assert_tool_not_found "${output}" x + assert [ ! -e "${marker}" ] +} + +@test "_run_cancel_hook: a tool__cancel executable on PATH is not run as the hook" { + local marker="${BATS_TEST_TMPDIR}/path-hook.ran" + _put_executable_on_path tool_x_cancel "${marker}" + + run _run_cancel_hook x '{}' + + assert_success + assert [ ! -e "${marker}" ] +}