fix: quote or refuse caller values reaching a shell, align bash conventions

[Martin Bens (SpiGAndromeda)]

Every MCP tool function takes a JSON arguments object from an untrusted caller, interpolates values into a command string, and runs that string through eval. Three of those values reached it with neither quoting nor a refusal. docker_service on the shopware-env lifecycle server was embedded bare, so a service name of web; id produced docker exec -i web; id bash -c '…' and ran as two commands. scope on the dev-tooling servers was interpolated into a jq filter program rather than passed as data, so a name of " // {"cwd":"$(printf PWNED)"} // .scopes."missing escaped the filter's string literal, was accepted where an ordinary undeclared name is refused, and left the command substitution sitting in SCOPE_CWD for wrap_command to place inside local double quotes. Separately, validate_tool_arguments returned success for any arguments value that was not a JSON object, so required, additionalProperties and enum were skipped on every tool of all four servers, including database_reset, whose run reaches system:install --drop-database. The branch began as a convention pass over the repository's bash and BATS against the software-writer skills and became the defects that pass surfaced. dev-tooling goes 3.17.1 to 3.17.2, shopware-env 1.2.4 to 1.2.5, and test-writing 4.2.4 to 4.2.5, the last two carrying the synced copies of the shared files.

Caller values that reach a command string

Every value that lands at the local level is now quoted by shell_quote_arg at construction, and a value that no quoting can carry is refused by assert_no_shell_hostile_chars before use. The division matters. The guard rejects only a single quote and a line break, because the docker, docker-compose and vagrant wrappers embed the whole command inside a single-quoted remote string, and those two characters are the ones no fixed escaping depth survives. Everything else is made safe by quoting rather than by rejection, which is why adding the guard alone left web; id working.

Value	Where it entered	Reached
docker_service, compose_file	resolve_lifecycle_env	four docker exec -i sites across environment.sh and docker-compose.sh, two of them the npm-path twins
plugin_name, plugin_namespace	tool_plugin_setup, tool_plugin_create	bin/console plugin:create and plugin:install, hand-wrapped in single quotes
scope	every dev-tooling tool taking it	five interpolated jq filters in scope.sh, plus the install_if_missing lookup in both JS servers through SCOPE_NAME
a jest.env value	a scope's config	KEY=value pairs joined by spaces ahead of the command, so NODE_OPTIONS=--require ./bootstrap.js executed ./bootstrap.js
compose_file	the compose module	-f ${file_path} unquoted, so a path containing a space split

tool_plugin_setup also validated nothing beyond non-emptiness. The PascalCase pattern its sibling applies lived inside tool_plugin_create alone.

Schema validation was skippable by sending a non-object

validate_tool_arguments ended its jq pipeline with || true. When arguments was a string, an array or null, $args | keys errored, the failure was masked, the message stayed empty, and the function returned success. Measured against a schema declaring required: ["force"] and additionalProperties: false: {"force":true} accepted, {} rejected, and "oops", [1,2] and null all accepted. A non-object is now rejected by name and type, and a jq failure is a rejection rather than a skip, on the grounds that a validator which cannot evaluate its input has not validated it. handle_tools_call also derived the object with .arguments // {}, and // treats a present null and a present false as absent, so both bypassed the new rejection until it became has("arguments").

A line break inside one path became two paths

parse_paths_json decoded with jq -r '.[]' and read the result back through a line-oriented loop, so a newline inside one element was indistinguishable from the separator between elements, and the hostile-character guard ran after the split and saw two clean fragments. One element of src/app\n. produced "src/app" ".", and . is the whole tree, so on eslint_fix, stylelint_fix and prettier_fix a single-path request wrote across the entire tree and reported success. That is the same outcome 3.17.0 already fixed once, reached by a different route. The top-level newline guard existed in all six PHP tool libs and none of the thirteen JS ones, so the tools whose whole purpose is path-scoping were the unprotected ones. The refusal now happens before the split.

ddev parses twice, and quoting cannot close it

shell_quote_arg escapes for exactly one parse. The docker, docker-compose and vagrant branches embed the command in a single-quoted remote string, and native has no remote shell, so one parse is the right target for four of the five environments. ddev emits bare argv, so the local eval consumes the escaping and ddev then joins the argv into bash -c inside the container. Measured against a model of ddev's own quoteArgs, one escaping layer executes a command substitution, two bake literal double quotes into every value, and three are a syntax error. No fixed depth is correct, because quoteArgs re-quotes only arguments containing " \t\r\n#, so the layer count depends on the value's own content and the sender cannot predict it. That is the condition the single-quote refusal already exists for, so the guard now refuses shell metacharacters when the environment is ddev. Globs stay allowed, since the container shell expanding them changes which files a tool sees but cannot execute caller text. This is a behavior change for ddev users, and it is reasoned from ddev's source rather than run against a live ddev project, which no machine here has.

A failed step reported success

tool_install_dependencies appended each exec_command result to its output, checked no status, and printed the accumulated text last, so a failed composer or npm install returned 0 with the error embedded in what read as a normal result, and the later steps ran anyway. tool_plugin_create's three steps and tool_plugin_setup's two behaved the same way, so a failed plugin:create still reached plugin:refresh and plugin:install. Nothing was going to catch these implicitly. mcpserver_core.sh dispatches every tool function on the left of ||, which disables errexit for the entire body and stops the ERR trap firing, so a status check written as cmd; rc=$? is dead code there. The same shape hid an awk failure in phpunit_coverage_gaps, which printed No files with uncovered lines. and returned 0 for a report it had never parsed, and it hid the eight PHP argument parsers whose || echo '{...}' fallback turned malformed caller JSON into a run against synthesized defaults.

.github/scripts broke in opposite bash versions

((failed++)) returns non-zero when the pre-increment value is 0, so under set -e on bash 5, which is what CI runs, the first failing check killed validate-issue-templates.sh and skipped every remaining check and the summary. An unguarded "${arr[@]}" on an empty array aborts under set -u on bash 3.2, which is macOS /bin/bash, and command_issue.yml's dropdown is empty today, so the same script aborted locally before printing anything. Both scripts now reach their summary under 3.2.57 and 5.3.15 with byte-identical output. discover-components.sh aborts on a malformed .mcp.json, but every consumer read it as done < <(discover_mcp_servers), and process substitution hides the producer's status from both $? and PIPESTATUS, so the validator could compare its dropdown against an empty server list and report it up to date. Fault-injected against a fixture tree holding one malformed file, the old shape continued with exit 0 and zero servers, losing the one good server it had already read.

What this does not close

npm_script_append_safe accepts a chain whose final segment is neither the tool nor a run-script, so eslint . && echo done is reported safe and the appended flags and paths land on echo while ESLint runs unscoped. The obvious fix, an expected-tool parameter matched against the script body, was implemented and reverted. It refuses the Administration's real build script, export VITE_MODE=production && ts-node -T build.ts, and lint:fix, npm run lint -- --fix. Nothing syntactic separates those from the hazard, and the caller knows only the npm script name, never the binary, so closing it needs a signal the function does not have. A KNOWN GAP block in the function's doc comment records both counterexamples.

Two fixes carry stated limits. The ddev refusal set is reasoned from ddev's source and a stand-in, never run against a live ddev project. The ${target_ids[@]+…} guard that stops an empty id array aborting under set -u before bash 4.4 was verified only by 3.2 aborting without it and 5.3 behaving correctly with it, since no 4.0 to 4.3 interpreter was available. The test-writing server's own floor rises to 4.4 in this branch, from the 4.2 declare -gA already required, because the globbing restore uses local -.

Tests

829 BATS tests pass, from 810 when the branch first went green and 819 after an independent review round. plugin-tests/mcp-shared/ gains the four suites whose subject is a template-owned module, moved out of plugin-tests/dev-tooling/ so a shared-module regression is no longer reported against one plugin's copy, and config_lsp_prefix.bats follows them as config.bats sourcing the template directly, which puts behavioral coverage behind shopware-env's copy for the first time.

The regression cases for the glob-expansion, jq-filter injection, path-splitting and discovery-producer defects were each run against the pre-fix code to confirm they fail there. That practice came from a failure rather than from discipline. The first CONV-* glob test passed against the unfixed get.sh, because bats never chdirs and nothing at the repository root matches CONV-*, so the glob had nothing to expand to and the case proved nothing. It now creates decoy files and runs with the process cwd among them, which is the shape the defect actually takes, since the server's cwd is the user's project root.

[github-actions]

Agent Skills Export

Built 7 skill packages:

• adr-writing
• feature-branch-pr-writing
• pr-description-writing
• ci-log-interpretation
• release-info-writing
• structuring-documentation
• commit-message-writing