kami/Maven
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

MCP client: connect out to external tool servers #70

Closed
claude wants to merge 1 commits from overnight/mcp-client into overnight/self-update
pull from: overnight/mcp-client
merge into: kami:overnight/self-update
Conversation 2 Commits 1 Files Changed 13
+2108 -6
claude commented 2026-08-01 02:23:19 +02:00
Contributor
Copy Link
Copy Source

What

The client half of docs/plans/06-mcp-support.md. Maven becomes an MCP host: she connects out to MCP servers and consumes their tools and resources. She is not an MCP server — nothing here exposes her own capabilities to an outside caller, because the plan does not ask for that direction.

New internal/mcp/:

  • jsonrpc.go / stdio.go / http.go — hand-rolled JSON-RPC 2.0 over two transports: a stdio subprocess on this box, and streamable HTTP that accepts either a plain JSON reply or an SSE frame (servers disagree about which they send; the Vikunja one sends SSE).
  • client.go — initialize handshake, tools/list, tools/call, resources/list, resources/read. Text content only.
  • manager.go — lazy dial, per-server failure that blocks neither boot nor the other servers, backoff reconnect, Status() for a web surface, Close().
  • allowlist.go — the encoding that lets the existing allowlist carry an MCP tool with no migration: row vikunja_list_tasks, cmd ["mcp","vikunja","list_tasks"], scope mcp:vikunja. ProposeTool, EnableTool, DisableTool, the act matcher and the confirm turn are untouched.
  • webfetchdoor.go — one guarded fetcher per url server.

internal/config: an mcp block (servers[] with command/args/env/dir or url, allow_private, allow_tools, max_tools, timeout, enabled). Absent, or with nothing enabled, normalises to nil. Bad blocks fail at startup, not at the first turn that needed the tool.

internal/webfetch: a Post method, because JSON-RPC cannot be a GET, plus response headers on Response for Mcp-Session-Id. It shares Get's guards exactly.

Why this shape

  • Off unless configured, and a described server stays dark until "enabled": true.
  • A url server goes through webfetch, so the SSRF guard, size cap, redirect cap and per-host rate limit apply to an MCP endpoint like they do to a news feed. Loopback needs allow_private on that server, and each server gets its own fetcher — one loopback exemption must not become a hole for a public endpoint that redirects.
  • readOnlyHint decides destructive. No hint means "assume it mutates", which routes the call through the confirm turn. Guessing wrong in that direction costs a question.
  • The catalogue stays small: allow_tools, and max_tools defaults to 12 per server. The resident model is a 1.7B with a 4096-token context; a tool name it half-remembers is a wrong act.
  • Discovery proposes, it does not enable. Kami enables on /tools, on the authed surface.
  • Only the tool name and the router's arguments are sent. There is no API here through which a note, a fact or the persona block could travel.

Verified

Against the real Vikunja MCP server on homesrv (http://localhost:9100/mcp):

mcp: vikunja connected (vikunja 0.1.0), 3 tool(s)
tool vikunja/get_task      readonly=true  local=vikunja_get_task
tool vikunja/list_projects readonly=true  local=vikunja_list_projects
tool vikunja/update_task   readonly=false local=vikunja_update_task
call err=<nil> out=[{"id":1,"title":"Homelab infra", ...
excluded tool err=mcp: vikunja offers no tool "delete_task"
no-allow-private: Err=... webfetch: refusing to connect to a private address: 127.0.0.1

Tests cover both transports (the stdio one against a real subprocess re-exec, no fixture file), SSE and JSON framing, session echo, the no-protocolVersion refusal, reconnect after failure, allow_tools/max_tools, and the config validation. make build and make test both green.

Vikunja #251

## What The client half of docs/plans/06-mcp-support.md. Maven becomes an MCP **host**: she connects out to MCP servers and consumes their tools and resources. She is not an MCP server — nothing here exposes her own capabilities to an outside caller, because the plan does not ask for that direction. New `internal/mcp/`: - `jsonrpc.go` / `stdio.go` / `http.go` — hand-rolled JSON-RPC 2.0 over two transports: a stdio subprocess on this box, and streamable HTTP that accepts either a plain JSON reply or an SSE frame (servers disagree about which they send; the Vikunja one sends SSE). - `client.go` — `initialize` handshake, `tools/list`, `tools/call`, `resources/list`, `resources/read`. Text content only. - `manager.go` — lazy dial, per-server failure that blocks neither boot nor the other servers, backoff reconnect, `Status()` for a web surface, `Close()`. - `allowlist.go` — the encoding that lets the existing allowlist carry an MCP tool with no migration: row `vikunja_list_tasks`, `cmd ["mcp","vikunja","list_tasks"]`, scope `mcp:vikunja`. `ProposeTool`, `EnableTool`, `DisableTool`, the act matcher and the confirm turn are untouched. - `webfetchdoor.go` — one guarded fetcher per url server. `internal/config`: an `mcp` block (`servers[]` with `command`/`args`/`env`/`dir` or `url`, `allow_private`, `allow_tools`, `max_tools`, `timeout`, `enabled`). Absent, or with nothing enabled, normalises to nil. Bad blocks fail at startup, not at the first turn that needed the tool. `internal/webfetch`: a `Post` method, because JSON-RPC cannot be a GET, plus response headers on `Response` for `Mcp-Session-Id`. It shares `Get`'s guards exactly. ## Why this shape - **Off unless configured**, and a described server stays dark until `"enabled": true`. - **A url server goes through webfetch**, so the SSRF guard, size cap, redirect cap and per-host rate limit apply to an MCP endpoint like they do to a news feed. Loopback needs `allow_private` on *that* server, and each server gets its own fetcher — one loopback exemption must not become a hole for a public endpoint that redirects. - **`readOnlyHint` decides `destructive`.** No hint means "assume it mutates", which routes the call through the confirm turn. Guessing wrong in that direction costs a question. - **The catalogue stays small**: `allow_tools`, and `max_tools` defaults to 12 per server. The resident model is a 1.7B with a 4096-token context; a tool name it half-remembers is a wrong act. - **Discovery proposes, it does not enable.** Kami enables on `/tools`, on the authed surface. - Only the tool name and the router's arguments are sent. There is no API here through which a note, a fact or the persona block could travel. ## Verified Against the real Vikunja MCP server on homesrv (`http://localhost:9100/mcp`): ``` mcp: vikunja connected (vikunja 0.1.0), 3 tool(s) tool vikunja/get_task readonly=true local=vikunja_get_task tool vikunja/list_projects readonly=true local=vikunja_list_projects tool vikunja/update_task readonly=false local=vikunja_update_task call err=<nil> out=[{"id":1,"title":"Homelab infra", ... excluded tool err=mcp: vikunja offers no tool "delete_task" no-allow-private: Err=... webfetch: refusing to connect to a private address: 127.0.0.1 ``` Tests cover both transports (the stdio one against a real subprocess re-exec, no fixture file), SSE and JSON framing, session echo, the no-`protocolVersion` refusal, reconnect after failure, `allow_tools`/`max_tools`, and the config validation. `make build` and `make test` both green. Vikunja #251
claude added 1 commit 2026-08-01 02:23:19 +02:00
Talk MCP: a client for external tool servers (#251) 95ae900a58 
docs/plans/06-mcp-support.md asks for the host direction — Maven connects OUT
to MCP servers and consumes what they offer. This is the client half: the
protocol, the transports, the connection manager, the config block. Nothing is
wired into a turn yet, and nothing here exposes Maven's own capabilities to an
outside caller.

internal/mcp:
  - hand-rolled JSON-RPC 2.0 (the wire format is four fields, and the repo
    vendors its deps, so a library would cost more than it saves);
  - two transports: a stdio subprocess on this box, and streamable HTTP, which
    accepts a plain JSON reply or an SSE frame because servers disagree about
    which they send;
  - Client: initialize handshake, tools/list, tools/call, resources/list,
    resources/read. Text content only — everything downstream is a sentence;
  - Manager: lazy dial, per-server failure that never blocks boot or the other
    servers, backoff reconnect, Status for a web surface, graceful Close;
  - the allowlist encoding: a discovered tool becomes the store row
    "vikunja_list_tasks" with cmd ["mcp","vikunja","list_tasks"], scope
    "mcp:vikunja". No new column, no migration, and ProposeTool, EnableTool,
    the act matcher and the confirm turn all keep working untouched.

Constraints held, in code rather than in prose:
  - OFF unless configured, and a server is dark until "enabled": true.
  - A url server goes through internal/webfetch, so the SSRF guard, the size
    cap, the redirect cap and the per-host rate limit apply. Reaching loopback
    needs allow_private on THAT server, and each server gets its own fetcher so
    one loopback exemption cannot become a hole for a public endpoint.
  - readOnlyHint decides destructive: no hint means "assume it mutates", which
    will route the call through the existing confirm turn. Guessing wrong in
    that direction only costs a question.
  - The catalogue stays small on purpose — allow_tools, and max_tools=12 per
    server. The resident model is a 1.7B with a 4096-token context; a tool name
    it half-remembers is a wrong act.
  - Only the tool name and the router's arguments are sent. There is no API
    here through which a note, a fact or the persona block could travel.

webfetch grows Post (JSON-RPC cannot be a GET) and surfaces response headers
for Mcp-Session-Id. It shares Get's guards exactly: a body buys a caller
nothing, a POST to the LAN is refused for the same reason a GET is.

Verified against the real Vikunja MCP server on homesrv
(http://localhost:9100/mcp): handshake, three discovered tools with update_task
correctly NOT read-only, a live list_projects call, a tool excluded by
allow_tools refused, and the same server refused outright once allow_private
was dropped. Tests cover both transports (the stdio one against a real
subprocess), SSE and JSON framing, session echo, reconnect, and the config
validation.
claude reviewed 2026-08-01 11:30:14 +02:00
claude left a comment
Author
Contributor
Copy Link
Copy Source

The trust shape is right and it is the part that is hardest to retrofit. One
webfetch.Fetcher per server instead of one shared one is the correct call:
allow_private for the Vikunja server on loopback cannot leak into another
server's public URL, and the comment on WebfetchDoor says why. Encoding an MCP
row as cmd: ["mcp", server, tool] avoids a store migration. ProposeTool, the
destructive flag and the confirm turn keep working unchanged. That is the
cheapest way to land this. Treating a missing readOnlyHint as
"assume it mutates" is the right default direction.

1. A silent stdio server wedges the entire manager, not just its own server

stdioTransport.Call holds t.mu across the read loop, and readLine blocks in
bufio.Reader.ReadString. That read never observes ctx. The ctx.Err() check at the top of the loop only fires between frames.
It is never reached when the child writes nothing at all. The 15s cfg.Timeout that Manager.Call
installs does nothing for this case.

Then it spreads. Manager.Refresh runs c.client.alive() while holding m.mu.
alive() goes to stdioTransport.alive(), which takes t.mu, which the hung
call still holds. Refresh now blocks forever with m.mu held, and Tools(),
Status(), Call() for every other server block behind it.

Walked through: a stdio server accepts stdin and stops writing. A python server
that hit an unhandled exception in its own read loop but did not exit is the
ordinary way to get there. Turn 1 calls its tool and hangs. The next daemon
tick calls Refresh and hangs. From that point Manager.Tools() never returns. Once PR 71 puts the discovered
catalogue on the act path, every turn hangs, including turns that touch no MCP
tool at all.

Two separate fixes are needed. Do the read on a goroutine and select on ctx,
so a call can abandon a silent pipe. And take the client pointers out of m.mu
before calling anything on them, in Refresh exactly as Resources already
does. Refresh is the one place in manager.go that calls into a client under
the lock.

2. maxLine does not bound anything

The comment says it bounds one JSON-RPC frame from a subprocess. The check is
len(line) > maxLine in readLine, and it runs after ReadString('\n') has
already assembled the whole line in memory. ReadString grows without limit,
the 64 KiB bufio buffer only bounds one syscall. A server that emits 500 MB
with no newline gets 500 MB allocated in mavend before the 1 MiB check rejects
it. On the deploy target that is an OOM kill of the core daemon.

The early-return path is worse. ReadString can return an error with a
non-empty partial line. That line is returned with no size check at all.

http.go gets this right with sc.Buffer(..., maxLine). io.LimitReader on
the stdout pipe, or a bufio.Scanner with the same Buffer call, gives stdio
the same property. Either way, the doc comment should stop claiming a bound the
code does not have.

3. decodeFrame will accept a request from the server as the response

The SSE branch keeps the last framed object with an id key. A JSON-RPC
request from the server has an id too. Sampling and roots/list are exactly
that. A server may send one mid-stream before it answers.
httpTransport.Call also never checks that the id it got back is the id it
sent, which the stdio transport does check.

Failure case: the server streams {"jsonrpc":"2.0","id":7,"method":"sampling/createMessage",...}
after the tool result frame. decodeFrame returns the sampling request.
Unmarshalled into rpcResponse it has no Result and no Error, so call
takes the len(resp.Result) == 0 early return and reports success with out
untouched. CallTool then returns ("", nil). An empty string and no error is the one answer that lies. The act is logged as
done, the tool never ran, and Maven speaks about an empty result.

Select on result or error being present, and match id against the request,
as stdio already does. The same gap makes ListTools return an empty catalogue
rather than an error against a server that behaves this way.

4. allow_private is total for that server, and a disabled server is never validated

Two smaller holes in the same area as the comment block.

WebfetchDoor sets c.AllowPrivate = cfg.AllowPrivate. That disables the
dialer guard for that fetcher entirely, on every hop. The comment argues a
loopback hole must not become a hole for a public endpoint that redirects at the
LAN. Across servers that holds. Within the server that has
the flag, it does not: http://localhost:9100/mcp responding 302 to
http://169.254.169.254/latest/meta-data/ is followed, up to MaxRedirects.
For a server reached over loopback, MaxRedirects: -1 on that fetcher costs
nothing and closes it. A local MCP endpoint has no business redirecting.

Second: Config.MCPServers() skips servers with enabled: false, and
validate() runs mcp.Validate on that filtered list. So a block with both
command and url, or a bare hostname as the url, passes startup validation
while it is dark. The doc on Enabled says a block can be written and reviewed before it is
switched on. The review the config layer could give is the one thing skipped. Validate the shape of every
configured server and let Enabled gate only the dialing.

Smaller notes

  • The per-host rate limit is now in a spoken turn's critical path. waitTurn
    blocks for HostInterval (1s default), and a dial is three requests to one
    host: initialize, the initialized notification, tools/list. That is 2s of
    pure sleeping per dial, and every later tools/call to that server pays up to
    1s before the request leaves. The limit was sized for a feed poll loop. An
    MCP server probably wants its own, much shorter, interval.
  • Tool.Description is taken verbatim, unbounded, from a server Maven does not
    control. Nothing here caps it. The resident model has 4096 tokens of context
    total. PR 71 has to defend this seam. The cap belongs
    here, next to the MaxTools cap that exists already.
  • filterTools sorts and then takes the first MaxTools. The comment calls
    that deterministic, and it is. It also hands the choice of which twelve to
    the server. A server that grows a thirteenth tool named aaa_ pushes a
    previously discovered tool out of the catalogue. Determinism was not the
    property worth buying. Preferring already-proposed rows, or refusing to trim
    at all without allow_tools, both keep the catalogue stable.
  • No way to send a credential to an HTTP server. Post takes hdr and
    httpTransport.send only ever puts Accept and the session id in it. A real remote MCP server needs a bearer token. The Vikunja one is reachable
    today only because it is unauthenticated on loopback.
  • LocalName can collide. Server vik, tool list_tasks and server
    vik_list, tool tasks both yield vik_list_tasks. Config-controlled, so
    low, but the store keys rows by name and the second proposal would land on the
    first row.
  • Untested: a stdio server that hangs, an SSE stream carrying a request from the
    server, an oversized frame, and a tools/list over MaxTools where the
    trimmed tail held an enabled row.
The trust shape is right and it is the part that is hardest to retrofit. One `webfetch.Fetcher` per server instead of one shared one is the correct call: `allow_private` for the Vikunja server on loopback cannot leak into another server's public URL, and the comment on `WebfetchDoor` says why. Encoding an MCP row as `cmd: ["mcp", server, tool]` avoids a store migration. ProposeTool, the destructive flag and the confirm turn keep working unchanged. That is the cheapest way to land this. Treating a missing `readOnlyHint` as "assume it mutates" is the right default direction. ## 1. A silent stdio server wedges the entire manager, not just its own server `stdioTransport.Call` holds `t.mu` across the read loop, and `readLine` blocks in `bufio.Reader.ReadString`. That read never observes `ctx`. The `ctx.Err()` check at the top of the loop only fires between frames. It is never reached when the child writes nothing at all. The 15s `cfg.Timeout` that `Manager.Call` installs does nothing for this case. Then it spreads. `Manager.Refresh` runs `c.client.alive()` while holding `m.mu`. `alive()` goes to `stdioTransport.alive()`, which takes `t.mu`, which the hung call still holds. Refresh now blocks forever with `m.mu` held, and `Tools()`, `Status()`, `Call()` for *every other server* block behind it. Walked through: a stdio server accepts stdin and stops writing. A python server that hit an unhandled exception in its own read loop but did not exit is the ordinary way to get there. Turn 1 calls its tool and hangs. The next daemon tick calls Refresh and hangs. From that point `Manager.Tools()` never returns. Once PR 71 puts the discovered catalogue on the act path, every turn hangs, including turns that touch no MCP tool at all. Two separate fixes are needed. Do the read on a goroutine and select on `ctx`, so a call can abandon a silent pipe. And take the client pointers out of `m.mu` before calling anything on them, in `Refresh` exactly as `Resources` already does. `Refresh` is the one place in manager.go that calls into a client under the lock. ## 2. `maxLine` does not bound anything The comment says it bounds one JSON-RPC frame from a subprocess. The check is `len(line) > maxLine` in `readLine`, and it runs after `ReadString('\n')` has already assembled the whole line in memory. `ReadString` grows without limit, the 64 KiB `bufio` buffer only bounds one syscall. A server that emits 500 MB with no newline gets 500 MB allocated in mavend before the 1 MiB check rejects it. On the deploy target that is an OOM kill of the core daemon. The early-return path is worse. `ReadString` can return an error with a non-empty partial line. That line is returned with no size check at all. `http.go` gets this right with `sc.Buffer(..., maxLine)`. `io.LimitReader` on the stdout pipe, or a `bufio.Scanner` with the same `Buffer` call, gives stdio the same property. Either way, the doc comment should stop claiming a bound the code does not have. ## 3. `decodeFrame` will accept a request from the server as the response The SSE branch keeps the last framed object with an `id` key. A JSON-RPC *request from the server* has an id too. Sampling and `roots/list` are exactly that. A server may send one mid-stream before it answers. `httpTransport.Call` also never checks that the id it got back is the id it sent, which the stdio transport does check. Failure case: the server streams `{"jsonrpc":"2.0","id":7,"method":"sampling/createMessage",...}` after the tool result frame. `decodeFrame` returns the sampling request. Unmarshalled into `rpcResponse` it has no `Result` and no `Error`, so `call` takes the `len(resp.Result) == 0` early return and reports success with `out` untouched. `CallTool` then returns `("", nil)`. An empty string and no error is the one answer that lies. The act is logged as done, the tool never ran, and Maven speaks about an empty result. Select on `result` or `error` being present, and match `id` against the request, as stdio already does. The same gap makes `ListTools` return an empty catalogue rather than an error against a server that behaves this way. ## 4. `allow_private` is total for that server, and a disabled server is never validated Two smaller holes in the same area as the comment block. `WebfetchDoor` sets `c.AllowPrivate = cfg.AllowPrivate`. That disables the dialer guard for that fetcher entirely, on every hop. The comment argues a loopback hole must not become a hole for a public endpoint that redirects at the LAN. Across servers that holds. Within the server that has the flag, it does not: `http://localhost:9100/mcp` responding 302 to `http://169.254.169.254/latest/meta-data/` is followed, up to `MaxRedirects`. For a server reached over loopback, `MaxRedirects: -1` on that fetcher costs nothing and closes it. A local MCP endpoint has no business redirecting. Second: `Config.MCPServers()` skips servers with `enabled: false`, and `validate()` runs `mcp.Validate` on that filtered list. So a block with both `command` and `url`, or a bare hostname as the url, passes startup validation while it is dark. The doc on `Enabled` says a block can be written and reviewed before it is switched on. The review the config layer could give is the one thing skipped. Validate the shape of every configured server and let `Enabled` gate only the dialing. ## Smaller notes - The per-host rate limit is now in a spoken turn's critical path. `waitTurn` blocks for `HostInterval` (1s default), and a dial is three requests to one host: initialize, the initialized notification, `tools/list`. That is 2s of pure sleeping per dial, and every later `tools/call` to that server pays up to 1s before the request leaves. The limit was sized for a feed poll loop. An MCP server probably wants its own, much shorter, interval. - `Tool.Description` is taken verbatim, unbounded, from a server Maven does not control. Nothing here caps it. The resident model has 4096 tokens of context total. PR 71 has to defend this seam. The cap belongs here, next to the `MaxTools` cap that exists already. - `filterTools` sorts and then takes the first `MaxTools`. The comment calls that deterministic, and it is. It also hands the choice of *which* twelve to the server. A server that grows a thirteenth tool named `aaa_` pushes a previously discovered tool out of the catalogue. Determinism was not the property worth buying. Preferring already-proposed rows, or refusing to trim at all without `allow_tools`, both keep the catalogue stable. - No way to send a credential to an HTTP server. `Post` takes `hdr` and `httpTransport.send` only ever puts `Accept` and the session id in it. A real remote MCP server needs a bearer token. The Vikunja one is reachable today only because it is unauthenticated on loopback. - `LocalName` can collide. Server `vik`, tool `list_tasks` and server `vik_list`, tool `tasks` both yield `vik_list_tasks`. Config-controlled, so low, but the store keys rows by name and the second proposal would land on the first row. - Untested: a stdio server that hangs, an SSE stream carrying a request from the server, an oversized frame, and a `tools/list` over `MaxTools` where the trimmed tail held an enabled row.
kami closed this pull request 2026-08-01 14:51:55 +02:00
kami commented 2026-08-01 14:52:49 +02:00
Owner
Copy Link
Copy Source

Landed on master. The stack was one linear chain, so #84 carried every commit from #50 up, and master now contains this branch in full. Merging this PR on its own is an empty diff, so it is closed rather than merged. The review findings for it were fixed in the 2026-08-01 pass and are on master as commits on the stack tip, not on this branch.

Landed on master. The stack was one linear chain, so #84 carried every commit from #50 up, and master now contains this branch in full. Merging this PR on its own is an empty diff, so it is closed rather than merged. The review findings for it were fixed in the 2026-08-01 pass and are on master as commits on the stack tip, not on this branch.

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
claude
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
kami
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kami/Maven#70
Delete Branch "overnight/mcp-client"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?