Repository navigation
fix!: answer -32603 when a configuration file is unreadable - #14
Merged
Merged
Conversation
read_json_file substituted {} for a missing file and passed an existing file through unparsed, so a broken MCP_CONFIG_FILE or MCP_TOOLS_LIST_FILE produced a healthy-looking answer: initialize fell back to its defaults and tools/list reported {"tools": []}, and a client could not tell a degraded result from a correct one. The function now requires a regular file holding exactly one parseable JSON document (null and false documents stay valid, which rules out jq -e) and returns 1 printing nothing, and initialize and tools/list turn that into a -32603 error naming the file.
validate_tool_arguments treats an unreadable tools list the same way: the old missing-file fallback to {} would have turned the new failure into a silent validation skip, so a missing or malformed list now rejects the call with the cannot-validate message, and a list that parses to something other than a tools object is rejected with its own message. Pre-existing suites that ran servers without configuration files now provide real fixtures; the new tests/read_json_file.bats pins the -32603 answers through process_request, and every regression case failed against the previous behavior first.
BREAKING CHANGE: MCP_CONFIG_FILE and MCP_TOOLS_LIST_FILE are effectively required. Servers that ran without one of them (or with an unparseable file) answered initialize with defaults and tools/list with an empty list; they now receive -32603, and tools/call is rejected as unvalidatable. Ship both files, valid, one JSON document each.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A shell holds one EXIT trap, so run_mcp_server's trap '_server_teardown' EXIT replaces the trap bats relies on to report a test when the loop runs in the bats test shell. While such a test passes nothing shows, but once its body aborts after the call, no TAP line is written and the test disappears from the run with only a count warning ("Executed N-1 instead of expected N tests"). A failing test that vanishes instead of failing. The one exposed call site, the direct-call test in tests/cancellation.bats, now runs the server in a subshell so the trap dies with it.
The new tests/exit_trap_isolation.bats pins both properties of the isolated shape: the caller's own EXIT trap survives the call, and the post-loop reset of _MCP_IN_SERVER_LOOP to 0 runs. The reset is observable only inside the subshell, since every guard reads the variable with a :-0 default that masks a deleted reset from the outside. The ownership contract is stated where it binds: at the trap site in lib/mcpserver_core.sh and in AGENTS.md, run_mcp_server takes over the process's EXIT trap and expects to be its last call; a caller that needs its own trap afterwards uses a subshell.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5.
read_json_filesubstituted{}for a missing file and passed an existing file through unparsed, so a brokenMCP_CONFIG_FILEorMCP_TOOLS_LIST_FILEproduced a healthy-looking answer:initializefell back to its defaults,tools/listreported{"tools": []}, and a client could not tell a degraded result from a correct one.Now:
read_json_filerequires a regular file holding exactly one parseable JSON document (nullandfalsedocuments stay valid, which rules outjq -e) and returns 1 printing nothing.initializeandtools/listturn a failed read into a-32603error naming the file.validate_tool_argumentsrejects a missing or malformed tools list instead of skipping validation — the old|| tools_config='{}'fallback would have turned the new failure into a silent validation bypass. A list that parses to something other than a tools object is rejected with its own message.Breaking (per AGENTS.md §Compatibility contract, major):
MCP_CONFIG_FILEandMCP_TOOLS_LIST_FILEare effectively required; servers running without them get errors instead of defaults. CHANGELOG carries the three Major entries.Tests: new
tests/read_json_file.batspins the-32603answers throughprocess_requestfor missing, malformed, empty, and multi-document files plus success andnull/falsecases; every regression case failed against the pre-fix code first. Pre-existing suites that ran servers without configuration files now use real fixtures.The second commit fixes a test hazard this change surfaced:
run_mcp_server'sEXITtrap replaced the trap bats uses to report a test when the loop ran in the test shell, so a test failing after that call vanished from the run count instead of printingnot ok. The one exposed call site now runs the server in a subshell,tests/exit_trap_isolation.batspins the isolation and the post-loop_MCP_IN_SERVER_LOOPreset, and the trap-ownership contract is documented at the trap site and in AGENTS.md.