From 205012f76e992bfcb76fb21e17f444cb62ad5ff1 Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Thu, 1 Oct 2026 20:22:15 +0200 Subject: [PATCH] feat: add a before-tool hook and a private directory per tool call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A server whose tools share a per-call step, such as an authorization check or an audit line, had to repeat it in every `tool_*` function, and a tool added later could skip it (#28). A tool that allocated a file for one call had no place for it that the SDK would clean up, and no stated contract for its own `trap … EXIT` (#27). `mcp_before_tool_call`, when the server defines it, runs before every dispatched tool with the tool name and the validated arguments, in the tool's own shell, so variables, the working directory, functions and traps it sets reach the tool. `_mcp_run_tool` saves and restores `set` and `shopt` options around it, and runs its own steps after the hook through `builtin` and bare redirections only, so a hook-defined function or a changed `PATH` cannot break them. A non-zero return refuses the call with an `isError` result carrying the hook's output. The markers `hook-running`, `hook-returned` and `hook-refused` let `handle_tools_call` tell a refusal, an `exit` and a signal apart, and each is logged as such. Each call now creates one root with `mktemp -d` (mode 700) holding `output`, `sentinel`, `hook-output`, the markers, and `tmp/`, which is exported as `MCP_CALL_TMPDIR` to the hook, the tool and the cancel hook. The in-flight record keeps its format. `_mcp_call_root_of` checks a path before any root is derived from it, and `_mcp_remove_call_root` removes only `mcp-call.*` directories after the call's group has ended, restoring write and search permission on directories the tool locked. A root that cannot be created answers `isError`, which also stops a failed `mktemp` from ending a server under `set -o posix`. In-process test captures that spawn a tool keep stderr out of the parsed JSON, because bash can print a `child setpgid` line there. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../software-writer/writing-code.md | 2 +- AGENTS.md | 2 + CHANGELOG.md | 7 + README.md | 49 +- docs/architecture.md | 52 +- lib/mcpserver_core.sh | 341 +++++++-- tests/before_tool_hook.bats | 603 +++++++++++++++ tests/call_tmpdir.bats | 686 ++++++++++++++++++ tests/cancellation.bats | 14 +- tests/lifecycle.bats | 97 ++- tests/tool_declaration.bats | 22 +- 11 files changed, 1788 insertions(+), 87 deletions(-) create mode 100644 tests/before_tool_hook.bats create mode 100644 tests/call_tmpdir.bats diff --git a/.claude/extensions/software-writer/writing-code.md b/.claude/extensions/software-writer/writing-code.md index f1200ba..547779b 100644 --- a/.claude/extensions/software-writer/writing-code.md +++ b/.claude/extensions/software-writer/writing-code.md @@ -18,7 +18,7 @@ - `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 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 "$output_file" 2>&1 _cancel` hook. The server removes it with its contents when the call returns, fails, is cancelled, or is in flight at a shutdown, including after the `SIGKILL` a tool that ignores `SIGTERM` gets. A server killed with `SIGKILL` leaves it behind under `TMPDIR` inside the call's `mcp-call.*` directory. A call whose directory cannot be created returns an `isError` result and does not run. +- `README.md` §Writing a server states that a tool's own `trap … EXIT` is supported. It runs when the tool returns and when the `SIGTERM` step of a cancellation or shutdown ends the tool, and not after a `SIGKILL`. + ### 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. @@ -16,6 +22,7 @@ All notable changes to this project are documented here. The format follows [Kee ### 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. +- A `tools/call` whose temporary files could not be created, for example because `TMPDIR` became unwritable after startup, ended a server running under `set -o posix` without a response. The status of the `mktemp` call was not checked. The call now returns an `isError` result and the server keeps running. ## [5.1.0] - 2026-09-15 diff --git a/README.md b/README.md index f3d31c1..2ae2f15 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,20 @@ Configured by environment variable before sourcing: | `MCP_EXTRA_LOG_FILE` | unset | Second log target; `PROJECT_ROOT` resolves a relative path. | | `MCP_LOG_STDERR` | `0` | Set to `1` to also mirror each log line to stderr. | +Defined by the server script and resolved as shell functions only: + +| Function | Called | +|---|---| +| `tool_` | For a `tools/call` naming ``, when `tools.json` declares it. Receives the `arguments` JSON. | +| `tool__cancel` | Optional. When a call of `` is cancelled, or the server shuts down with it in flight. | +| `mcp_before_tool_call` | Optional. Before every dispatched tool, with the tool name and the `arguments` JSON. | + +Set by the SDK for a call, not by the consumer: + +| Variable | Seen by | Meaning | +|---|---|---| +| `MCP_CALL_TMPDIR` | `mcp_before_tool_call`, the tool function and what it starts, `tool__cancel` | A directory private to the call, removed when the call ends. See *Per-call files* below. | + A missing or unparseable `MCP_CONFIG_FILE` or `MCP_TOOLS_LIST_FILE` is not read as an empty configuration; both files are effectively required. `initialize` and `tools/list` answer `-32603`, naming the file they could not read. That covers a file whose single document is not a JSON object. A `tools/call` whose tools list cannot be read is rejected with an `isError` result instead of being dispatched unvalidated. Methods handled: `initialize`, `tools/list`, `tools/call` and `ping`; the notifications `notifications/initialized` and `notifications/cancelled`. A request for any other method returns `-32601`; a notification for one is logged and ignored. @@ -117,10 +131,11 @@ A tool that exits non-zero returns its combined output as an `isError` result ra > [!IMPORTANT] > Stdout is the protocol channel. A tool function's stdout becomes the tool result, so everything else a server wants to say goes through `log`. A stray `echo` outside a tool corrupts the stream. -Two properties of tool dispatch to write against: +Three properties of tool dispatch to write against: - A tool function always runs with errexit disabled, on every dispatch path. That holds under `run_mcp_server` and from a direct call to `process_request` or `handle_tools_call` alike. A failing step does not end the tool. Check each step's status yourself and return non-zero to produce the `isError` result. - A tool function's stdin is `/dev/null`. A read returns EOF instead of blocking on the server's protocol stream or consuming bytes meant for it. +- A tool function may install a `trap … EXIT`. It runs when the tool returns, before the result is built, so anything it prints joins the tool result. It also runs when the `SIGTERM` step of a cancellation or a server shutdown ends the tool. It does not run when the call's process group is killed with `SIGKILL`, which is what a tool that ignores `SIGTERM` gets after the grace. A file that must not outlive the call belongs in `MCP_CALL_TMPDIR` (*Per-call files* below). A tool cannot leave a value in a variable for a later call to read. Each call runs in its own subshell, so what a tool sets dies with that call (the boundary is in `docs/architecture.md` §Request lifecycle and §Tool containment). State that crosses calls travels through a file. @@ -137,6 +152,34 @@ Inside a tool, `$$` is the server's pid while `BASHPID` is the per-call subshell Removing the file is the server script's job. *Cancelling and shutting down* below gives the traps `run_mcp_server` installs, the subshell the server script uses to keep its own, and what a `SIGKILL` leaves behind. +### Running a step before every tool + +Tools that share a per-call step, such as an authorization check, an audit line or a change of working directory, can define it once as `mcp_before_tool_call`. When a shell function of that name exists, every dispatched `tools/call` runs it before the tool function. An executable or alias of that name is never run. + +It receives the tool name as `$1` and the call's `arguments` JSON as `$2`, exactly as the tool function receives it. It runs only for a call that passed the tools-list check and argument validation, so an undeclared tool or rejected arguments never reach it. + +The hook and the tool function run in one shell, the call's own subshell. A global variable the hook assigns, an exported variable, a function it defines and a directory it changes to are what the tool sees. The two also share one `EXIT` trap, so a `trap … EXIT` in the tool replaces one the hook installed. A function the hook defines reaches the tool function but not the SDK's own steps after the hook. Those steps use only `builtin`-prefixed shell builtins and plain redirections, so a function the hook defines and a `PATH` it changes do not affect them. A hook must not define a function named `builtin`: the SDK's steps call `builtin` by that name and would run the hook's function instead. + +Shell options are the exception. Whatever the hook sets with `set` or `shopt` is put back once it returns, so the tool runs with the options it would have had without the hook. A hook may turn on `set -euo pipefail` for its own body, and the tool still runs with errexit off. + +A hook that returns 0 lets the tool run, and its stdout and stderr are discarded. A hook that returns non-zero stops the call. The tool does not run, and the call returns an `isError` result whose text is the hook's combined output, prefixed `Error executing : ` as for a failing tool. The log records it as `before-tool hook refused tool (status N)`, not as a tool failure. + +Write the hook with `return`, not `exit`. An `exit` ends the call's shell instead of returning to it, so the tool does not run whatever status the hook exits with. The call returns an `isError` result that names the hook, followed — when the hook printed anything — by what it printed. `exit 0` gets that result too, rather than an empty success. A hook killed by a signal also stops the call, and the result names the signal. Both cases are logged at `ERROR`. + +The hook runs under the same dispatch properties as a tool function: errexit off, stdin `/dev/null`, and inside the call's process group, so a cancellation stops a hook that blocks. A nested `handle_tools_call` or `process_request` made from inside a tool runs the hook again for the inner call. + +### Per-call files + +Each dispatched call gets a directory of its own, exported as `MCP_CALL_TMPDIR` to the before-tool hook, the tool function and every process the tool starts. It is created empty with mode `700`, and no other call shares it. Its path is absolute even when `TMPDIR` is relative. + +It sits in the call's root, a directory `mcp-call.*` under `TMPDIR` (default `/tmp`) with mode `700`. The root also holds the files the server keeps for the call: its collected output and the server's own bookkeeping. They are not part of the contract, and a tool should write only inside `MCP_CALL_TMPDIR`. + +The server removes the root, and `MCP_CALL_TMPDIR` with it, once the call's process group has exited or been killed. That happens when the tool returns, when it fails, when the call is cancelled, and when the server shuts down with the call in flight. It covers the `SIGKILL` a tool gets after ignoring `SIGTERM`, which no `EXIT` trap survives, so a file the call keeps there needs no cleanup of its own. A subdirectory the call left read-only, or with no permissions at all, is removed too. A server killed with `SIGKILL` is the exception (*Cancelling and shutting down* below). A removal that fails is logged at `WARN`. It neither ends the server nor changes the call's outcome: a call that gets a response still gets it, and a cancelled call still gets none. + +A call whose root or `MCP_CALL_TMPDIR` cannot be created does not run. It returns an `isError` result instead, and the failure is logged at `ERROR`. + +A nested dispatch inside a tool gets a directory of its own. Its `MCP_CALL_TMPDIR` applies only inside that inner call. + ### Cancelling and shutting down A client cancels an in-flight call by sending `notifications/cancelled` with the request's id in `params.requestId`. The server stops the tool's process group with `SIGTERM`, then `SIGKILL` for whatever is left of it, and sends no response for that id. A cancellation that names nothing in flight is logged and dropped. A tool that dies on the `SIGTERM` is reaped at once rather than after the two-second grace (measured as in `docs/architecture.md` §Cancellation). @@ -147,6 +190,8 @@ A tool that must outlive the call has to leave the group itself by detaching int A tool may define an optional `tool__cancel` hook. It runs with the call's original `arguments` JSON as its one argument. A signal that tears the server down passes an empty string instead: the in-flight record holds the tool's group and name, but no arguments. +The hook gets the call's `MCP_CALL_TMPDIR` on both routes, so it can read what the call recorded there, such as the path of a file the call created or the id of a job it started. The directory still exists while the hook runs. + The hook runs before the group is signalled, and each of the two steps gets its own two seconds. A hook that runs longer than two seconds is killed, and its exit status is logged rather than failing the call. A wedged hook followed by a group that ignores `SIGTERM` therefore holds a cancellation for about four seconds before the final `SIGKILL`. `run_mcp_server` installs `EXIT`, `INT`, `TERM`, `HUP` and `PIPE` traps. The four signal traps replace any handler a consumer set on those signals, because a signal handler that expects the shell to keep running cannot be honored: the shell has to die by the signal. The `EXIT` trap is the server's own, and a consumer's `EXIT` handler is not lost with it: the server runs that handler as part of its teardown, after its own cleanup, on a clean exit and on a trapped signal alike. @@ -163,7 +208,7 @@ Kill the process group to stop a call and still see its result. To stop the serv Bash cannot install a handler for a signal that was ignored when the shell started. A non-interactive shell's plain `&` hands the background job `SIGINT` and `SIGQUIT` already ignored. A server backgrounded that way with job control off therefore has no `INT` trap at all. The `TERM` and `HUP` traps install normally. -A server killed with `SIGKILL` runs no trap, so a call it had in flight is stopped by the lifeline instead (the lifeline and its sentinel are in `docs/architecture.md` §The lifeline). That kill runs no cleanup, so small files under `TMPDIR` (default `/tmp`) can be left behind, and nothing removes them. The patterns are `mcp-inflight.*`, `mcp-lifeline.*`, `mcp-tool-output.*`, `mcp-sentinel.*` and `mcp-partial.*`. A server that goes down on a signal it can trap removes the in-flight call's output file as part of its teardown. +A server killed with `SIGKILL` runs no trap, so a call it had in flight is stopped by the lifeline instead (the lifeline and its sentinel are in `docs/architecture.md` §The lifeline). That kill runs no cleanup, so small files under `TMPDIR` (default `/tmp`) can be left behind, and nothing removes them. The patterns are `mcp-inflight.*`, `mcp-lifeline.*`, `mcp-call.*`, `mcp-partial.*` and `mcp-shutdown.*`, where `mcp-call.*` is a call's root with its `MCP_CALL_TMPDIR` and whatever the call left in it. A server that goes down on a signal it can trap removes the in-flight call's root as part of its teardown. A client that closes stdin mid-call still gets that call's response. The server finishes the call, answers it, and then exits. Requests that arrived during the call are answered after it, in order. diff --git a/docs/architecture.md b/docs/architecture.md index 94a15da..387f00a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -51,18 +51,53 @@ The tool then runs in the background under `set -m`, which gives the job its own The wrapper redirects to the call's output file, merges stderr into it, and takes its stdin from `/dev/null`. An inherited stdin would be the client's JSON-RPC stream, and a tool reading it would consume protocol bytes. The wrapper inherits no lifeline descriptor either, because `process_request` closed the dispatch subshell's copy on the way in. Both the wrapper and the sentinel open the lifeline FIFO by path, read-only. +Before the spawn, `handle_tools_call` creates the call root with `mktemp -d` under `TMPDIR`, named `mcp-call.*`. `mktemp -d` creates it exclusively and with mode 700, so no other user can place anything at a name inside it, and every file of the call is written there with a plain `>`. A relative `TMPDIR` produces a relative root, which is made absolute so that a hook that changes directory still reaches it. + +| Path in the root | Holds | Written by | +|---|---|---| +| `output` | The wrapper's stdout and stderr | The wrapper's redirection | +| `sentinel` | The lifeline sentinel's pid, on the server loop only | The wrapper | +| `tmp` | `MCP_CALL_TMPDIR`, created with `mkdir -m 700` | `handle_tools_call` | +| `hook-output` | `mcp_before_tool_call`'s stdout and stderr; left in the root, which the answer for a hook that called `exit` reads it out of, and removed with the root | `_mcp_run_tool` | +| `hook-running` | Nothing; present from before the hook runs until the root is removed | `_mcp_run_tool` | +| `hook-returned` | Nothing; present once the hook has returned, whatever its status | `_mcp_run_tool` | +| `hook-refused` | Nothing; present once the hook has returned non-zero | `_mcp_run_tool` | + +A root that cannot be created, or a `tmp` that cannot, fails the call: the error is logged, whatever was created is removed, an `isError` result answers, and nothing is spawned. Each status is checked, so a failed `mktemp -d` never leaves a path relative to the working directory. + +Inside the wrapper, `_reset_tool_dispatch_state` runs first. The wrapper then exports `MCP_CALL_TMPDIR` and calls `_mcp_run_tool`. When `declare -F` finds `mcp_before_tool_call`, that function saves the shell options from `set +o` and `shopt -p`, creates `hook-running`, and runs the hook in a brace group redirected to `hook-output`. A brace group does not fork, so the hook's assignments, functions, working directory and `EXIT` trap stay in the wrapper's shell for the tool. Once the hook returns, the saved options are restored and `hook-returned` is written, whatever the status. None of the steps that follow the hook reaches for an external command — the restore runs `builtin set` and `builtin shopt`, and every marker is a bare redirection — so a hook that rewrites `PATH` cannot strand a marker. The tool therefore runs with errexit off even after a hook that turned it on. + +A non-zero hook status creates `hook-refused`, prints the capture to stdout, which is the call's output file, and becomes the wrapper's exit status. A zero status leaves the capture unread and runs the tool; the capture file stays in the root either way, and goes when the root does. No hook means no marker at all. + +`handle_tools_call` reads the markers when it collects a call that was not cancelled. + +| Root holds | Wrapper status | Answer | Log | +|---|---|---|---| +| `hook-running` without `hook-returned` | 128 or below | `isError` naming the hook as having ended the call, followed by the captured output when there is any | `ERROR` naming the hook and the status | +| `hook-running` without `hook-returned` | above 128 | `isError` naming the signal that killed the hook | `ERROR` naming the hook and the status | +| `hook-refused` | non-zero | `isError` with the hook's output, as for a failing tool | `before-tool hook refused tool (status N)` | +| otherwise | non-zero | `isError` with the tool's output | `Tool failed with exit code N` | +| otherwise | 0 | the tool's output | none | + +A `hook-running` with no `hook-returned` means the hook never returned: it called `exit`, or a signal killed the wrapper. Whatever status the hook exited with is discarded. + The process group is the containment boundary. | Member | Enters the group | Leaves it | |---|---|---| -| The wrapper subshell | `set -m` makes it the leader, and its pid is the group id | Exits when the tool function returns | +| The wrapper subshell | `set -m` makes it the leader, and its pid is the group id | Exits when the tool function returns, or when the hook returns non-zero or calls `exit` | +| The before-tool hook | Runs inside the wrapper, before the tool | Returns before the tool runs; a non-zero return or an `exit` ends the wrapper | | The tool function | Runs inside the wrapper | Exits with the wrapper | | The lifeline sentinel | The wrapper starts it before the tool | Waits on the lifeline, so it outlives the tool; the group SIGKILL reaps it | | Anything the tool started and did not wait for | Inherits the group | The group SIGKILL reaps it | `_kill_tool_group` runs after the wrapper is reaped and clears whatever is left. It signals only a group that still holds a live member, because an emptied group id can already belong to something else. -`_reset_tool_dispatch_state` runs inside the wrapper, immediately before the tool. It clears `_MCP_IN_SERVER_LOOP` and unsets the three file variables. A tool is free to call `process_request` or `handle_tools_call` for a nested dispatch of its own. +Once a call is dispatched, its root is removed at three sites, each after the group has exited or been killed. `handle_tools_call` removes it once the wait has returned, for a call that was not cancelled. `_teardown_tool_call` removes it after its `_kill_tool_group`, which covers the cancelled call. `_server_teardown` removes the directory of the in-flight record's output path (§Shutdown). A call whose root or `tmp` cannot be created is never dispatched, and that path removes only the root it created. A server killed with `SIGKILL` reaches none of these sites. + +Every site removes the root through `_mcp_remove_call_root`, which returns at once for a path that is already gone. Before it touches anything it refuses a path that is not absolute or whose basename is not `mcp-call.*`, logging the refusal at `WARN`, and `_server_teardown` requires the output path it derives the root from to end in `output`. It runs `rm -rf` first. When the root survives, `find` without `-L` restores read, write and search permission on each directory that lacks one, with a per-directory `-exec … \;` so each directory is fixed before `find` reads it, and `rm -rf` runs again. `-type d` matches no symlink, so the target of a symlink in the root keeps its mode. A root that still survives is logged at `WARN` with the captured errors, and the function returns 0. A removal failure does not change the call's outcome: a call that gets a response still gets it, and a cancelled call still gets none. Under `set -o posix` a failing command in the dispatch would otherwise end the server. + +`_reset_tool_dispatch_state` runs inside the wrapper, ahead of the hook and the tool. It clears `_MCP_IN_SERVER_LOOP` and unsets the three file variables. A tool is free to call `process_request` or `handle_tools_call` for a nested dispatch of its own. Without that reset, the nested dispatch would poll the tool's `/dev/null` stdin. It would read the instant EOF as a client closing the stream and set the shutdown flag. That stops the server as soon as the outer call returns. Only the wrapper subshell's copies change, so the outer call keeps its own state and its own in-flight record. @@ -96,9 +131,9 @@ The record is written after the spawn, and it is best effort. A teardown that la A pgid that an emptied group once held can be handed to another process group. A teardown reading a numeric id there would TERM and KILL a group that has nothing to do with this call. The tombstone takes that id out of the record while keeping the tool name and the output path, which a truncation would lose. -`_server_teardown` reads the pgid field first. A `-` means signal nothing, run no cancel hook, poll no grace, and release only the two named files. A numeric field means a call is still running. +`_server_teardown` reads the pgid field first. A `-` means signal nothing, run no cancel hook, poll no grace, and release only the call root the output path names. A numeric field means a call is still running. -The sentinel pid file is not in the record. `_sentinel_pid_file` derives it from the output file's path, since that path is the only handle a shutdown teardown has on the call. +The call root is not in the record. Every reader takes it as the directory of the output path, `${output_file%/*}`, and finds the sentinel pid file and `MCP_CALL_TMPDIR` in it as `sentinel` and `tmp`. That path is the only handle a shutdown teardown has on the call. ## Cancellation @@ -116,7 +151,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 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. +`_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. Its third argument is the call's output file, whose directory is the call root, and it exports that root's `tmp` to the hook as `MCP_CALL_TMPDIR`. With no output path it unsets the variable instead, so a hook never sees an empty value. Both callers run it before the group is signalled and before the call root is removed. 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. @@ -130,7 +165,7 @@ That handler is captured in `run_mcp_server`, immediately before the traps are i `_server_teardown` is idempotent, because every step is a no-op once its subject is gone. A `_MCP_TEARDOWN_RUNNING` flag keeps a second entry out while a pass is running. The flag is cleared on the way out, so a shell that runs a second server still tears that one down. -A pass stops the in-flight call, then releases files, then drops the lifeline. It removes the call's output file and sentinel pid file, the in-flight record, the shutdown flag, and the partial-line file. It closes the lifeline descriptor and removes the lifeline directory; dropping the last writer is what releases a sentinel still waiting. Each variable is cleared with the file it names, so a second pass cannot remove a path the shell has since given to something else. +A pass stops the in-flight call, then releases files, then drops the lifeline. It removes the in-flight call's root, the in-flight record, the shutdown flag, and the partial-line file. It closes the lifeline descriptor and removes the lifeline directory; dropping the last writer is what releases a sentinel still waiting. Each variable is cleared with the file it names, so a second pass cannot remove a path the shell has since given to something else. A signal that arrives mid-pass is recorded rather than re-raised. `_mcp_teardown_on_signal` stores it in `_MCP_PENDING_SIGNAL` and returns. Re-raising there would end the shell before the pass reached its SIGKILL and its file removals. The finishing pass restores the signal's default disposition and re-raises it against `BASHPID`, so the shell still dies by the signal. @@ -140,7 +175,7 @@ Bash runs a trap only between foreground commands, and that sets the two signal A signal to the whole process group kills the dispatch subshell at once. The command substitution returns, and the trap reaps a tool group that is still alive. -A server killed with `SIGKILL` runs no teardown, so its temporary files stay under `TMPDIR`. Those are the in-flight record (`mcp-inflight.*`), the lifeline directory (`mcp-lifeline.*`), a running call's output file (`mcp-tool-output.*`) and sentinel pid file (`mcp-sentinel.*`), and the partial-line file (`mcp-partial.*`). The lifeline sentinel still kills the tool group; only the files are left behind. +A server killed with `SIGKILL` runs no teardown, so its temporary files stay under `TMPDIR`. Those are the in-flight record (`mcp-inflight.*`), the lifeline directory (`mcp-lifeline.*`), a running call's root (`mcp-call.*`), the partial-line file (`mcp-partial.*`), and the shutdown flag (`mcp-shutdown.*`). The lifeline sentinel still kills the tool group; only the files are left behind. ## Partial-line handoff @@ -169,6 +204,9 @@ The branch also sets `_MCP_EOF_DRAIN`. `handle_tools_call` then touches the shut | 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 | +| A tool never runs without a call directory of its own | `handle_tools_call` | §Tool containment | +| A tool runs only after `mcp_before_tool_call`, when defined, returns 0 | `_mcp_run_tool` | §Tool containment | +| A call root is removed only after its call's group has exited or been killed | `handle_tools_call`, `_teardown_tool_call`, `_server_teardown` | §Tool containment | | A tool group dies even when the server could run no trap | `_lifeline_sentinel` | §The lifeline | | A process group is signalled only while it still holds a live member | `_kill_tool_group`, `_tool_group_has_live_member` | §The in-flight record | | A cancelled call gets no response for its id | `_await_tool_call` | §Cancellation | diff --git a/lib/mcpserver_core.sh b/lib/mcpserver_core.sh index 3637a2b..a63ded7 100755 --- a/lib/mcpserver_core.sh +++ b/lib/mcpserver_core.sh @@ -774,12 +774,42 @@ handle_tools_call() { # cancelled by process group. Job-control notices exist only for interactive # shells, so the `set -m` here adds nothing to stderr. # docs/architecture.md §Tool containment. - local output_file - output_file=$(mktemp "${TMPDIR:-/tmp}/mcp-tool-output.XXXXXX") - # Where the wrapper below records the sentinel's pid; _sentinel_pid_file - # carries the derivation and its reason. - local sentinel_file - sentinel_file="$(_sentinel_pid_file "$output_file")" + # Every file of the call lives in one root that `mktemp -d` creates mode 700 + # and exclusively, so no other user can place anything at a name inside it + # and the files below are written with a plain `>`. + # docs/architecture.md §Tool containment. + local call_root root_rc=0 + call_root=$(mktemp -d "${TMPDIR:-/tmp}/mcp-call.XXXXXX" 2>&1) || root_rc=$? + if [[ $root_rc -ne 0 ]]; then + log "ERROR" "Tool $tool_name not run: cannot create its call directory in ${TMPDIR:-/tmp}: ${call_root}" + local setup_result + setup_result=$(jq -n -c \ + --arg text "Cannot run ${tool_name}: its call directory could not be created." \ + '{"content": [{"type": "text", "text": $text}], "isError": true}') + create_response "$id" "$setup_result" + return + fi + # A relative TMPDIR yields a relative root, and a hook that changes + # directory would then miss its own capture file and hand the tool a + # MCP_CALL_TMPDIR that names nothing from where it runs. + [[ $call_root == /* ]] || call_root="$PWD/$call_root" + # The in-flight record holds the output file's path alone, so every reader + # derives the root as its directory; the names below are that contract. + local output_file="$call_root/output" + local sentinel_file="$call_root/sentinel" + local call_tmpdir="$call_root/tmp" + local mkdir_error mkdir_rc=0 + mkdir_error=$(mkdir -m 700 -- "$call_tmpdir" 2>&1) || mkdir_rc=$? + if [[ $mkdir_rc -ne 0 ]]; then + log "ERROR" "Tool $tool_name not run: cannot create MCP_CALL_TMPDIR ${call_tmpdir}: ${mkdir_error}" + _mcp_remove_call_root "$call_root" + local setup_result + setup_result=$(jq -n -c \ + --arg text "Cannot run ${tool_name}: its call directory could not be created." \ + '{"content": [{"type": "text", "text": $text}], "isError": true}') + create_response "$id" "$setup_result" + return + fi local pid local monitor_was_enabled=0 if [[ -o monitor ]]; then @@ -797,13 +827,15 @@ handle_tools_call() { # wrapper that cannot write it leaves the group's raw liveness as the # teardown's only measure, which is slower but not wrong. # docs/architecture.md §Tool containment. - ( exec {lifeline_rd}<"${_MCP_LIFELINE_DIR}/lifeline"; _lifeline_sentinel "${lifeline_rd}" & sentinel_pid=$!; set +e; printf '%s\n' "$sentinel_pid" > "${sentinel_file}"; _reset_tool_dispatch_state; "$func_name" "$arguments" ) \ + # shellcheck disable=SC2030 # MCP_CALL_TMPDIR is exported for this call's wrapper alone; the dispatch shell must not keep it. + ( exec {lifeline_rd}<"${_MCP_LIFELINE_DIR}/lifeline"; _lifeline_sentinel "${lifeline_rd}" & sentinel_pid=$!; set +e; printf '%s\n' "$sentinel_pid" > "${sentinel_file}"; _reset_tool_dispatch_state; export MCP_CALL_TMPDIR="$call_tmpdir"; _mcp_run_tool "$func_name" "$tool_name" "$arguments" "$call_root" ) \ >"$output_file" 2>&1 "$output_file" 2>&1 "$output_file" 2>&1 _cancel` executable found on PATH. @@ -1108,6 +1179,16 @@ _run_cancel_hook() { return 0 fi + local call_root="" + local call_tmpdir="" + if [[ -n "$output_file" ]]; then + if _mcp_call_root_of call_root "$output_file"; then + call_tmpdir="${call_root}/tmp" + else + log "WARN" "refusing to derive a call root from ${output_file}: not a call root; cancel hook ${hook} runs with MCP_CALL_TMPDIR unset" + fi + fi + local hook_pid local waited=0 local hook_status=0 @@ -1116,7 +1197,11 @@ _run_cancel_hook() { monitor_was_enabled=1 fi set -m - ( if [[ -n "${_MCP_LIFELINE_FD:-}" ]]; then exec {_MCP_LIFELINE_FD}>&-; fi; set +e; "$hook" "$arguments" ) >/dev/null 2>&1 &-; fi; set +e; if [[ -n "$call_tmpdir" ]]; then export MCP_CALL_TMPDIR="$call_tmpdir"; else unset MCP_CALL_TMPDIR; fi; "$hook" "$arguments" ) >/dev/null 2>&1 /mcp-call.*`. Anything else — a root a stale or + # corrupted in-flight record named — is refused before `rm` or `find` runs, + # so it cannot turn the removal loose on an arbitrary directory. + if [[ $root != /* || ${root##*/} != mcp-call.* ]]; then + log "WARN" "refusing to remove ${root}: not a call root" + return 0 + fi + + # _teardown_tool_call may have removed it: an absent path is not a failure. + [[ -e $root || -L $root ]] || return 0 + + rm_error=$(rm -rf -- "$root" 2>&1) || rm_rc=$? + if [[ -e $root || -L $root ]]; then + # No `--` end-of-options marker: BusyBox `find` may read it as a path, + # and every call site passes an absolute path, which cannot be an option. + chmod_error=$(find "$root" -type d ! -perm -u+rwx -exec chmod u+rwx {} \; 2>&1) || true + rm_rc=0 + rm_error=$(rm -rf -- "$root" 2>&1) || rm_rc=$? + fi + if [[ $rm_rc -ne 0 || -e $root || -L $root ]]; then + log "WARN" "Cannot remove call directory ${root}: ${rm_error}${chmod_error:+ (restoring permissions: ${chmod_error})}" + fi + return 0 +} + +# Run a tool function behind the consumer's optional mcp_before_tool_call hook, +# inside the call's wrapper subshell. Neither the hook nor the tool is forked +# off: both run in the wrapper's shell, so a global variable the hook assigns, +# a directory it changes to, and an EXIT trap it installs reach the tool. The +# shell options are the exception: those the hook sets are put back before the +# tool runs, and so are functions: what the hook defines reaches the tool but +# not the steps this function runs once the hook has returned. +# README.md §Running a step before every tool. +# Args: $1 = tool function name, $2 = tool name, $3 = the call's arguments JSON, +# $4 = the call root, which holds the hook's capture file and markers +# Outputs: a failed hook's captured output on stdout, which the wrapper has +# pointed at the call's output file; otherwise the tool's own output. +# Returns: the hook's status when it is non-zero, else the tool's status +_mcp_run_tool() { + local func_name="$1" + local tool_name="$2" + local arguments="$3" + local call_root="$4" + + # `declare -F` matches a shell function only, as for the tool itself: an + # executable or alias named mcp_before_tool_call never runs as the hook. + if declare -F mcp_before_tool_call >/dev/null; then + local hook_status=0 + local saved_set saved_shopt + saved_set=$(set +o) + saved_shopt=$(shopt -p) + # The marker outlives the wrapper only when the hook never returned — + # it called `exit` or a signal killed the wrapper — which is what + # handle_tools_call answers for once the wrapper is reaped. A bare + # redirection, so no command name the hook could shadow — `:` included — + # is reached and no PATH lookup happens. + # shellcheck disable=SC2188 # a commandless redirection is the point: any command name here is one the hook may have shadowed or taken off PATH. + >"$call_root/hook-running" + # A brace group, not a subshell: its redirection applies without a fork, + # so what the hook sets survives into the tool call below. The capture + # keeps a successful hook's output out of the tool result. + { mcp_before_tool_call "$tool_name" "$arguments"; } >"$call_root/hook-output" 2>&1 || hook_status=$? + # Every step below runs after the hook returned, so each one bypasses + # what the hook may have changed: a function it defined named `set`, + # `shopt` or `printf` must not take over the SDK's own steps, and a PATH + # it rewrote must not decide whether those steps can run. Nothing below + # reaches for an external command — each file is written with a bare + # redirection and the one command run is a builtin — so a hook that sets + # PATH=/nonexistent leaves no marker behind. The restore re-executes the + # snapshot with every command prefixed `builtin` for the same reason, + # since the snapshot's own `set` and `shopt` calls would otherwise + # resolve to the hook's functions. The tool call at the end is + # deliberately not bypassed: a function the hook defines reaches the + # tool. README.md §Running a step before every tool. + # Restored before anything else runs, so an errexit or nounset the hook + # turned on governs neither the steps below nor the tool. + builtin eval "builtin ${saved_set//$'\n'/$'\n'builtin }" + builtin eval "builtin ${saved_shopt//$'\n'/$'\n'builtin }" + # Written whatever the hook's status, and before the refusal branch: this + # plus `hook-running` is what tells handle_tools_call the hook returned + # rather than died. It is left in place with `hook-output`; the whole + # root goes when the call does. + # shellcheck disable=SC2188 # a commandless redirection is the point: any command name here is one the hook may have shadowed or taken off PATH. + >"$call_root/hook-returned" + if [[ $hook_status -ne 0 ]]; then + # Tells handle_tools_call that the non-zero status is the hook's + # refusal, not the tool's failure. + # shellcheck disable=SC2188 # a commandless redirection is the point: any command name here is one the hook may have shadowed or taken off PATH. + >"$call_root/hook-refused" + builtin printf '%s' "$(<"$call_root/hook-output")" + builtin return "$hook_status" + fi + fi + "$func_name" "$arguments" } # The pid recorded in a call's sentinel file. Empty when no record was written — @@ -1261,12 +1474,15 @@ _teardown_tool_call() { local id="$4" local output_file="$5" - _run_cancel_hook "$tool_name" "$arguments" + _run_cancel_hook "$tool_name" "$arguments" "$output_file" - local sentinel_file - sentinel_file="$(_sentinel_pid_file "$output_file")" - local sentinel_pid - sentinel_pid="$(_sentinel_pid_for "$sentinel_file")" + local call_root="" + local sentinel_pid="" + if _mcp_call_root_of call_root "$output_file"; then + sentinel_pid="$(_sentinel_pid_for "${call_root}/sentinel")" + else + log "WARN" "refusing to derive a call root from ${output_file}: not a call root" + fi local alive_waited=0 if kill -0 -- "-$pid" 2>/dev/null; then @@ -1287,7 +1503,11 @@ _teardown_tool_call() { # which is what a cancellation is expected to look like. wait "$pid" || true _kill_tool_group "$pid" "$tool_name" "$output_file" - rm -f -- "$output_file" "$sentinel_file" + # A path that named no call root has no root to remove, and the check above + # already logged the refusal. + if [[ -n "$call_root" ]]; then + _mcp_remove_call_root "$call_root" + fi log "INFO" "Cancelled tools/call $id ($tool_name)" } @@ -1484,10 +1704,16 @@ _server_teardown() { local inflight_pid="" local inflight_tool="" local inflight_output="" + local inflight_root="" local inflight_sentinel_file="" if [[ -n "${_MCP_INFLIGHT_FILE:-}" && -s "${_MCP_INFLIGHT_FILE}" ]]; then read -r inflight_pid inflight_tool inflight_output < "${_MCP_INFLIGHT_FILE}" || true - inflight_sentinel_file="$(_sentinel_pid_file "${inflight_output}")" + fi + # The record is not trusted to name a call: the root, and the sentinel path + # beside it, are derived only from an output path this SDK minted. Every + # read and every removal below is aimed through that one checked root. + if _mcp_call_root_of inflight_root "${inflight_output}"; then + inflight_sentinel_file="${inflight_root}/sentinel" fi # A pgid field of `-` is the tombstone: _kill_tool_group wrote it as it @@ -1498,9 +1724,9 @@ _server_teardown() { # behaviour. if [[ -n "${inflight_pid}" && "${inflight_pid}" != "-" ]] \ && kill -0 -- "-${inflight_pid}" 2>/dev/null; then - _run_cancel_hook "${inflight_tool}" "" - # The sentinel this call's wrapper recorded beside the output file is - # left out of the group's liveness exactly as in _teardown_tool_call: it + _run_cancel_hook "${inflight_tool}" "" "${inflight_output}" + # The sentinel this call's wrapper recorded in the call root is left + # out of the group's liveness exactly as in _teardown_tool_call: it # ignores TERM and waits on the lifeline, so counting it would hold this # teardown for the whole grace on every shutdown. local inflight_sentinel @@ -1519,14 +1745,21 @@ _server_teardown() { log "INFO" "Stopped in-flight tool ${inflight_tool} (group ${inflight_pid}) on shutdown" fi - # The tool's output file is named nowhere but the dispatch subshell that - # created it, so a server going down on a signal has only the record to - # reach it; the sentinel pid file beside it is derived from that same path - # and is removed with it. This removal runs for a tombstone record too, - # which is the point of keeping the path in it. A teardown that found no - # call to stop leaves nothing to remove either way. + # The call root is named nowhere but the dispatch subshell that created it, + # so a server going down on a signal has only the record to reach it: the + # root is the directory of the output file the record names, and it is + # removed with everything in it after the group above has been stopped. + # This removal runs for a tombstone record too, which is the point of + # keeping the path in it. A teardown that found no call to stop leaves + # nothing to remove either way. + # The root removed here is the one checked above, so a record naming a path + # outside a call root leaves that path's parent alone. if [[ -n "${inflight_output}" ]]; then - rm -f -- "${inflight_output}" "${inflight_sentinel_file}" + if [[ -n "${inflight_root}" ]]; then + _mcp_remove_call_root "${inflight_root}" + else + log "WARN" "refusing to remove ${inflight_output%/*}: not a call root" + fi fi # The variables are cleared with the files they name: a later step in this diff --git a/tests/before_tool_hook.bats b/tests/before_tool_hook.bats new file mode 100644 index 0000000..04ac843 --- /dev/null +++ b/tests/before_tool_hook.bats @@ -0,0 +1,603 @@ +#!/usr/bin/env bats +# bats file_tags=mcp-core,before-tool-hook +# Pins the optional mcp_before_tool_call hook a server may define. When it is a +# shell function, every dispatched tools/call runs it first, in the tool's own +# shell, with the tool name and the validated arguments JSON: +# - it runs with errexit off and stdin at /dev/null, as the tool does; +# - a hook that returns 0 has its output discarded, and what it sets — a +# variable, the working directory, an EXIT trap — reaches the tool, while +# a shell option it sets does not; +# - a function the hook defines reaches the tool, but not the SDK's own steps +# after the hook returns, and neither does a PATH the hook rewrites: those +# steps reach no external command; +# - a hook that returns non-zero stops the call with an isError result that +# carries the hook's output, the tool does not run, and the log records a +# refusal rather than a tool failure; +# - a hook that ends the wrapper shell with `exit` instead of returning stops +# the call the same way, whatever status it exits with: the tool does not +# run, and the isError result carries the hook's captured output when it +# printed any; +# - a hook killed by a signal stops the call with an isError result naming +# the signal, and a wrapper killed while no hook is defined is answered as +# the tool's failure, never as the hook's; +# - a call that is not dispatched — an undeclared tool, arguments that fail +# validation — does not run the hook, and neither does an executable on +# PATH of that name when no function is defined; +# - a nested handle_tools_call inside a tool runs the hook again; +# - a cancellation stops a hook that blocks. +# Each "did not run" claim is proved by the marker file the hook or the tool +# would have written. Requests are driven through process_request; the +# cancellation case drives a throwaway server over the FIFO client harness. +# process_request runs under `run --separate-stderr`: bash can report a failed +# job-control setpgid for the wrapper on stderr, and that line is not part of +# the response the assertions parse. +bats_require_minimum_version 1.11.0 + +load "${BATS_TEST_DIRNAME}/test_helper/common_setup" +load "${BATS_TEST_DIRNAME}/test_helper/mcp_client" +load "${BATS_TEST_DIRNAME}/test_helper/write_server" + +setup() { + MCP_LOG_FILE="${BATS_TEST_TMPDIR}/server.log" + MCP_EXTRA_LOG_FILE="" + MCP_CONFIG_FILE="${BATS_TEST_TMPDIR}/config.json" + 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 + + printf '%s\n' '{}' > "${MCP_CONFIG_FILE}" + printf '%s\n' '{ + "tools": [ + {"name": "x", "inputSchema": {"type": "object", "properties": {"n": {"type": "integer"}}, "additionalProperties": false}}, + {"name": "nested", "inputSchema": {"type": "object"}}, + {"name": "inner", "inputSchema": {"type": "object"}} + ] + }' > "${MCP_TOOLS_LIST_FILE}" + + # shellcheck source=../lib/mcpserver_core.sh + source "${REPO_ROOT}/lib/mcpserver_core.sh" +} + +teardown() { + mcp_stop_server + unset MCP_LOG_FILE MCP_EXTRA_LOG_FILE MCP_CONFIG_FILE MCP_TOOLS_LIST_FILE PROJECT_ROOT \ + HOOK_SLEEP_PID_FILE TOOL_MARKER MCP_SERVER_INSTANCE +} + +# A tools/call line for tool with and request id , +# 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" + local id="${3:-1}" + jq -nc --arg n "${name}" --argjson a "${arguments_json}" --argjson id "${id}" \ + '{jsonrpc: "2.0", id: $id, method: "tools/call", params: {name: $n, arguments: $a}}' +} + +# Assert is a result for id 1 with isError whose text is +# exactly . +_assert_result_text() { + local response="$1" + local is_error="$2" + local text="$3" + run jq -e --argjson e "${is_error}" --arg t "${text}" \ + '.id == 1 and .result.isError == $e and .result.content[0].text == $t and (.error == null)' <<< "${response}" + assert_success +} + +# Count the running processes of the server this test started, the way +# tests/lifecycle.bats counts the fixture's: every subshell the SDK forks keeps +# the server's argv, so the script path and the per-start instance marker +# select them all and nothing else. +_server_procs() { + local script="$1" + local marker="--mcp-test-instance=${MCP_SERVER_INSTANCE:-}" + local line + local count=0 + while IFS= read -r line; do + if [[ "${line}" == Z* ]]; then + continue + fi + if [[ "${line}" == *"${script}"* && "${line}" == *"${marker}"* ]]; then + count=$(( count + 1 )) + fi + done < <(ps -A -o stat=,args=) + printf '%s' "${count}" +} + +# Wait up to for to hold something. +_wait_for_file() { + local file="$1" + local limit="$2" + local deadline=$(( SECONDS + limit )) + while (( SECONDS < deadline )); do + if [[ -s "${file}" ]]; then + return 0 + fi + sleep 0.05 + done + return 1 +} + +# Wait up to for process to be gone or a zombie. +_wait_for_exit() { + local pid="$1" + local limit="$2" + local deadline=$(( SECONDS + limit )) + local state="" + while (( SECONDS < deadline )); do + state="$(_mcp_proc_state "${pid}")" + if [[ -z "${state}" || "${state}" == Z* ]]; then + return 0 + fi + sleep 0.05 + done + return 1 +} + +# Wait up to for exactly processes of server