Repository navigation
feat: adopt MCP servers through a proxy tool and a generated skill - #353
Merged
Merged
Conversation
Nothing the model reads mentioned MCP: the frozen pointer named no surface
for it and no --spec contract covered it, so a request to add an MCP server
left the agent to improvise a JSON-RPC client for each server.
openmax --mcp-list and --mcp-call are a one-shot MCP stdio client outside the
agent loop. Each run starts the server named after --, performs the
initialize handshake with protocol-version negotiation, pages through
tools/list or makes one tools/call, bounds every wait with --mcp-timeout,
passes the server's stderr through, and stops the server (stdin closed, then
SIGTERM, then SIGKILL). openmax --spec mcp prints the recipe built on it: one
proxy tool per server with a {tool, arguments} schema and the server's
credentials granted through env, plus one skill whose body --mcp-list
generates, so tool schemas cost nothing until the skill is read. Each call
spawns the server; servers that need a persistent session are not supported
yet.
The frozen pointer now names mcp among its surfaces: 4 more bytes on every
request (3,416, under the unchanged 3,500 cap).
openmax --mcp-call wrote each request to the server's stdin with a blocking write from the thread that waits for replies. A server that answered initialize and then stopped reading held any request larger than the pipe buffer before --mcp-timeout started counting, so the proxy tool hung until the server exited on its own. Writes now go through a writer thread fed by a queue, so the timeout bounds every wait. Stopping the server closes the queue, and the SIGTERM that follows frees a writer still blocked on a full pipe. The stop sequence moves into Drop, removing a guard that could never fire. The bridge test target's own main read the value of libtest options such as --test-threads 1, and the pattern after --skip, as name filters, so those command lines ran none of its tests, or only the skipped one, and still reported ok. It now parses them as libtest does. --spec mcp also names the quoted server instructions that --mcp-list prints.
The extending guide said each bridge run makes one request, but `--mcp-list` against a paginating server sends one tools/list request per page and follows nextCursor until the last one. The sentence now names what a run sends: every page of tools/list, or one tools/call.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+396
to
+404
| if self.exited_within(STOP_GRACE) { | ||
| return; | ||
| } | ||
| #[cfg(unix)] | ||
| { | ||
| if let Ok(pid) = libc::pid_t::try_from(self.child.id()) { | ||
| // SAFETY: the pid is this process's own unreaped child, so it | ||
| // cannot have been reused by another process. | ||
| unsafe { libc::kill(pid, libc::SIGTERM) }; |
There was a problem hiding this comment.
If an MCP server wrapper starts a worker, cleanup checks and signals only the direct child, then returns when that child exits. Both --mcp-list and --mcp-call can finish successfully while the worker remains running with inherited environment and file descriptors. Stop the owned process tree on both cleanup paths before merging.
Knowledge Base Used: Tool execution and execution controls
Artifacts
- The authored script runs the real CLI against an MCP wrapper that spawns a descendant, records PID states, and terminates its own fixture processes.
- The executed fixture captured four successful CLI runs and showed each wrapper gone while its descendant was still alive.
- The same execution captured explicit cleanup of all four owned descendants, showing they were stopped by the validation script rather than openmax.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/tui/src/mcp.rs
Line: 396-404
Comment:
**Server descendants survive cleanup**
If an MCP server wrapper starts a worker, cleanup checks and signals only the direct child, then returns when that child exits. Both `--mcp-list` and `--mcp-call` can finish successfully while the worker remains running with inherited environment and file descriptors. Stop the owned process tree on both cleanup paths before merging.
**Knowledge Base Used:** [Tool execution and execution controls](https://app.greptile.com/trace-labs/-/custom-context/knowledge-base/max17190/open-max/-/docs/tool-execution-and-fallback.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+599
to
+605
| if items.is_empty() { | ||
| return match result.get("structuredContent") { | ||
| Some(structured) if !structured.is_null() => structured.to_string(), | ||
| _ => String::new(), | ||
| }; | ||
| } | ||
| items |
There was a problem hiding this comment.
When a tool returns nonempty content alongside structuredContent, this branch prints only the content items. Fields supplied solely in the structured result never reach the proxy or model, so a successful call can omit information needed to use its result. Preserve the structured result alongside nonempty content before merging.
Artifacts
- This authored script serves a result containing both fields and runs the CLI against the reviewed source and an isolated corrected build, making the two captures reproducible.
- The command built and called the PR checkout through the stdio fixture; exit code 0 and stdout containing only the text marker show that the structured value was dropped.
- The command built an isolated worktree with the focused rendering change and called the same fixture; exit code 0 and stdout containing both markers show the consequence of the fix.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/tui/src/mcp.rs
Line: 599-605
Comment:
**Structured results are discarded**
When a tool returns nonempty `content` alongside `structuredContent`, this branch prints only the content items. Fields supplied solely in the structured result never reach the proxy or model, so a successful call can omit information needed to use its result. Preserve the structured result alongside nonempty content before merging.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.…lformed results The bridge queued every line a server wrote in an unbounded channel, so a server looping on notifications could grow the bridge without limit while a call waited or while the server was being stopped. The reader now hands over one line at a time through a zero-capacity channel, and a flooding server blocks on its own stdout instead. --mcp-call read at most 64 MiB of stdin and parsed what it got, so a call cut at the bound could still parse and run without its tail. It now reads one byte past the bound and refuses larger input with exit 2 before any server starts. A tools/call result whose content was not an array printed nothing and exited 0, reporting a malformed reply as an empty, completed call; it is now reported as malformed and exits 1.
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.
Why
An agent asked to use an MCP server had no supported way to do it. Open Max hosts no MCP client in the agent loop, so the agent was left to write its own JSON-RPC client in a shell script, with no handshake, no timeout, and no clean shutdown of the server process. Hosting a client in the loop would instead put every server's tool schemas into every request.
This change lets the agent adopt any MCP stdio server through the two surfaces it already writes: one proxy tool and one generated skill. Every request pays only for the proxy's small
{tool, arguments}schema and the skill's index line. The server's tool list is read only when the skill is, and nothing beyond the server itself needs to be installed.Summary
openmax --mcp-list -- <server command...>prints the server's tools as a skill body: a header, the server's instructions as a quoted block, then one line per tool such as- name(arg: type, opt?: type): description.--jsonprints the full definitions.openmax --mcp-call -- <server command...>reads{"tool", "arguments"}on stdin and prints the result's text. It runs outside the agent loop, with no session, endpoint, or trust state.notifications/initialized, protocol revisions 2024-11-05 through 2025-11-25. A server answering with any other revision is refused, and the message names both versions.tools/listis paged by cursor and stops on a repeated cursor. While waiting, the client answers server pings, refuses other server requests with -32601, and ignores notifications.--mcp-timeout(default 30, 1 to 3600 seconds). Writes to the server go through a separate writer thread, so a server that stops reading cannot stall a large call past the timeout. On exit the server's stdin is closed, then SIGTERM and SIGKILL follow 2 seconds apart, and the process is reaped. Server stderr passes through. Output lines are capped at 64 MiB, and the reader takes one line at a time, so a server that floods its output blocks on its own stdout rather than growing the bridge.--mcp-timeoutjoin the single flag-refusal table. Every word after--is kept as the server's own argv, sonpx -y pkgis never parsed as Open Max options.--keeps its old meaning when no MCP operation is given. The help text lists the new flags.openmax --spec mcp. A recipe for the proxy TOML tool (credentials granted throughenv,mutating = true,timeout_secs = 120) and the shell lines that write the skill's SKILL.md from--mcp-list. It states that each call starts the server and that servers needing a persistent session are not supported yet.mcp. The frozen payload grows by 4 bytes, from 3,412 to 3,416 bytes (770 to 773 tokens). It stays under the existing 3,500-byte cap, and the gate's comment records the new size.docs/extending.mdhas a new "MCP servers" section and an updated surface list.docs/usage.mdadds the new commands.Test Plan
crates/tui/tests/mcp_bridge.rsis a custom-harness test binary that runs itself as a fixture MCP server, because a standard test harness prints to stdout, which is the server's protocol channel. It has 8 tests:a_proxy_tool_written_from_the_spec_lists_and_calls_through_openmax: builds the skill from the spec's own shell block and the proxy from its own TOML block, passesopenmax --check, then has a headless model turn call the proxy. It asserts that the result reaches the model, that only the granted env var reaches the server, and that server stderr passes through.an_is_error_result_exits_nonzero_with_its_text,a_malformed_reply_is_reported_and_exits_nonzero: covers error results, error replies, bad input (including a call that parses when cut at the 64 MiB bound but not whole), malformed replies (including a result whose content is a number), and an unsupported protocol version.a_server_that_never_answers_times_out_and_is_stopped,a_server_that_stops_reading_cannot_stall_a_large_call: covers the timeout bound, including a 1 MiB call to a server that never reads, and confirms the server process is gone afterwards.every_page_of_tools_is_listed: three pages, in both text and--jsonoutput.a_server_request_is_answered_and_a_notification_ignored: a ping and a notification arrive in the middle of a call.this_harness_reads_libtest_options_as_libtest_does: pins test selection for--skip,--exact,--ignored, and the value-taking options.the_mcp_recipe_tool_loads_as_a_tool_and_arguments_proxyparses the spec's TOML through the real tool parser.the_reader_reads_no_further_than_one_line_aheadfeeds the reader a stream of notifications and pins that it reads at most one line beyond what the session has taken, and that every line still arrives.the_mcp_server_command_is_every_word_after_the_separator,an_mcp_operation_without_a_server_command_is_refused, andevery_command_the_mcp_spec_shows_is_acceptedcover argument parsing, and the refusal-table tests now include the new operations.npx: listing, calls, image content, error results, progress notifications, and no leftover processes.cargo test --workspace --lockedpasses, includingfrozen_prompt_fits_token_budgetandthe_guide_pointer_names_only_real_surfaces.cargo +1.97.0 clippy --workspace --all-targets --locked -- -D warningspasses.Not safe to merge until the two outstanding MCP issues are fixed.
Findings
Fix with agent prompt
Summary
The PR adds a one-shot MCP stdio bridge, CLI support, an adoption recipe, tests, and documentation. The latest changes also make session, ledger, and trust locks release explicitly when their guards drop. Two previously reported MCP issues remain unresolved: server descendants can survive cleanup, and structured results can be omitted from tool output.
Reviews (4) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..."