From 8d5e357b576c9add23cb979542293e0c4717c35b Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 04:36:40 +0400 Subject: [PATCH] Expose discovered MCP tools through the act allowlist (#251) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second half of the MCP client: the tools the manager discovers become rows in the existing act allowlist instead of a parallel capability system. An MCP tool is encoded in the columns that already exist — cmd ["mcp",,], scope mcp: — so no migration, and ProposeTool/EnableTool/DisableTool, tool.Matcher and the confirm turn need no changes. One branch in Executor.Exec routes such a row to the manager instead of exec, and "mcp" is never run as a binary. Discovery only ever PROPOSES. destructive comes from the inverse of the MCP readOnlyHint, so a tool that does not promise to be read-only inherits the confirm turn, and enabling stays on /tools behind step-up. Voice args are positional and MCP args are named, so CallPositional binds only what it can defend: no required properties runs bare, and a read-only tool with exactly one required string or number gets the tail. Everything else refuses with ErrNeedsArgs rather than guessing. The read-only condition was learned against the live Vikunja server: update_task requires only task_id and takes the rest as optional, so one guessed argument blanked the fields it did not mention. A partially-filled write destroys what it omits, so a mutating tool never receives a guessed argument. Also: a read-only mcp_servers IPC method and an "MCP servers" card on /tools showing transport, target and state, with the trust level of a local target spelled out. There is deliberately no call-a-tool IPC method and no run button, so mutation keeps exactly one path. Vikunja #251 --- cmd/mavend/actions_act.go | 7 ++ cmd/mavend/main.go | 15 ++++ cmd/mavend/mcp.go | 154 ++++++++++++++++++++++++++++++++++ cmd/mavend/mcp_test.go | 77 +++++++++++++++++ cmd/mavend/tick.go | 11 +++ cmd/mavend/voicewire.go | 13 +++ cmd/mavweb/handlers_test.go | 54 ++++++++++++ cmd/mavweb/main.go | 29 ++++++- deploy/mavend.json | 14 ++++ internal/ipc/api.go | 21 +++++ internal/ipc/client.go | 9 ++ internal/ipc/server.go | 14 ++++ internal/ipc/unimplemented.go | 3 + internal/ipc/wire.go | 1 + internal/mcp/manager.go | 92 ++++++++++++++++++++ internal/mcp/mcp_test.go | 119 ++++++++++++++++++++++++++ internal/store/tools.go | 39 +++++++++ internal/store/tools_test.go | 62 ++++++++++++++ internal/tool/tool.go | 33 ++++++++ internal/tool/tool_test.go | 100 ++++++++++++++++++++++ 20 files changed, 863 insertions(+), 4 deletions(-) create mode 100644 cmd/mavend/mcp.go create mode 100644 cmd/mavend/mcp_test.go diff --git a/cmd/mavend/actions_act.go b/cmd/mavend/actions_act.go index d5a1dc1..be83e9e 100644 --- a/cmd/mavend/actions_act.go +++ b/cmd/mavend/actions_act.go @@ -5,6 +5,7 @@ import ( "errors" "log" + "github.com/kami/maven/internal/mcp" "github.com/kami/maven/internal/router" "github.com/kami/maven/internal/tool" ) @@ -52,6 +53,12 @@ func (h *reactiveHandler) actionAct(ctx context.Context, dec router.Decision) st return "выполнить «" + phrase + "»? скажи «да» или «нет»." case errors.Is(err, tool.ErrNotEnabled): return h.proposeGap(ctx, dec) + case errors.Is(err, mcp.ErrNeedsArgs): + // An MCP tool that wants named arguments a spoken verb cannot + // supply. Guessing them would be a wrong act, so she says so + // instead — the tool is still runnable from the authed surface, + // where a human types them. + return "этому инструменту нужны аргументы, которые я из голоса не соберу — я не буду угадывать." } log.Printf("voice: tool %s: %v", dec.Slots.Fn, err) if out != "" { diff --git a/cmd/mavend/main.go b/cmd/mavend/main.go index e4fffa2..9253338 100644 --- a/cmd/mavend/main.go +++ b/cmd/mavend/main.go @@ -280,6 +280,9 @@ func run(args []string) error { api := coreAPI.(*daemonAPI) api.chatFn = voiceW.handler.handleText } + if voiceW != nil && voiceW.mcp != nil { + coreAPI.(*daemonAPI).getMCPServers = voiceW.mcp.status + } } else { // locked mode: no real store yet, so there's no meaningful CoreAPI to // serve. srv.Check below is the actual guard — every CoreAPI call is @@ -518,6 +521,11 @@ func run(args []string) error { }() } + // Keep MCP connections alive (nil unless configured). + if voiceW != nil && voiceW.mcp != nil { + go voiceW.mcp.run(ctx) + } + dl.unlock() log.Printf("mavend: unlocked via passkey assertion") return nil @@ -577,6 +585,13 @@ func run(args []string) error { crawlWkr.run(ctx) }() } + if voiceW != nil && voiceW.mcp != nil { + wg.Add(1) + go func() { + defer wg.Done() + voiceW.mcp.run(ctx) + }() + } } <-ctx.Done() diff --git a/cmd/mavend/mcp.go b/cmd/mavend/mcp.go new file mode 100644 index 0000000..7b31117 --- /dev/null +++ b/cmd/mavend/mcp.go @@ -0,0 +1,154 @@ +package main + +import ( + "context" + "fmt" + "log" + "time" + + "github.com/kami/maven/internal/config" + "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/mcp" + "github.com/kami/maven/internal/store" + "github.com/kami/maven/internal/webfetch" +) + +// mcpRefreshInterval — how often the manager re-dials a server that is down. +// The manager applies its own backoff on top, so this being short is cheap. +const mcpRefreshInterval = time.Minute + +// mcpWiring — the MCP client, when the `mcp` block configures at least one +// enabled server. nil ⇒ nothing was configured, nothing is connected, and an +// allowlist row that happens to look like an MCP row refuses to run. +// +// It lives on the voice wiring because MCP tools ARE acts: they run through +// tool.Executor, the enabled allowlist and the confirm turn, which only exist +// on the voice/chat path. No voice surface ⇒ nothing that could call a tool. +type mcpWiring struct { + mgr *mcp.Manager + st *store.Store +} + +// wireMCP builds the manager, connects, and proposes what it found. It never +// fails the daemon: a server that is unreachable at boot is logged and retried, +// because Maven starting is not contingent on someone else's process. +func wireMCP(cfg *config.Config, st *store.Store) *mcpWiring { + servers := cfg.MCPServers() + if len(servers) == 0 { + return nil + } + limits := webfetch.Config{} + if cfg.MCP != nil { + limits.AllowHosts = cfg.MCP.AllowHosts + limits.DenyHosts = cfg.MCP.DenyHosts + limits.MaxBytes = cfg.MCP.MaxBytes + limits.Timeout = time.Duration(cfg.MCP.Timeout) + } + mgr, err := mcp.NewManager(mcp.WebfetchDoor(limits), servers) + if err != nil { + // Validation already ran in config.validate, so this is a programming + // error rather than a config one. Still not fatal: MCP off is a working + // Maven. + log.Printf("mcp: not wired: %v", err) + return nil + } + w := &mcpWiring{mgr: mgr, st: st} + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + mgr.Connect(ctx) + w.propose(ctx) + return w +} + +// propose writes a 'proposed' allowlist row for every discovered tool. It does +// NOT enable anything: a configured server is a place Maven may look, not a +// capability she has. Kami enables what he wants on /tools, behind step-up, +// which is the same gate a shell tool goes through. +// +// Re-running on every boot is idempotent — ProposeMCPTool never touches an +// existing row, so a tool he disabled stays disabled and one he enabled keeps +// the cmd he enabled it with. +func (w *mcpWiring) propose(ctx context.Context) { + if w == nil { + return + } + now := time.Now() + fresh := 0 + for _, t := range w.mgr.Tools() { + name := mcp.LocalName(t.Server, t.Name) + // No readOnlyHint ⇒ assume it mutates ⇒ the confirm turn. Being wrong + // in this direction only costs a question. + destructive := !t.ReadOnly + provenance := fmt.Sprintf("mcp %s/%s", t.Server, t.Name) + if t.Description != "" { + provenance += ": " + t.Description + } + ok, err := w.st.ProposeMCPTool(ctx, name, mcp.Scope(t.Server), + mcp.Cmd(t.Server, t.Name), destructive, provenance, now) + if err != nil { + log.Printf("mcp: propose %s: %v", name, err) + continue + } + if ok { + fresh++ + } + } + if fresh > 0 { + log.Printf("mcp: %d new tool proposal(s) waiting on /tools", fresh) + } +} + +// run re-dials downed servers and picks up tools that appeared, until ctx is +// canceled. +func (w *mcpWiring) run(ctx context.Context) { + if w == nil { + return + } + t := time.NewTicker(mcpRefreshInterval) + defer t.Stop() + for { + select { + case <-ctx.Done(): + return + case <-t.C: + w.mgr.Refresh(ctx) + w.propose(ctx) + } + } +} + +// status maps the manager's view onto the wire type the web surface reads. +func (w *mcpWiring) status() []ipc.MCPServerStatus { + if w == nil { + return nil + } + in := w.mgr.Status() + out := make([]ipc.MCPServerStatus, 0, len(in)) + for _, s := range in { + out = append(out, ipc.MCPServerStatus{ + Name: s.Name, + Transport: s.Transport, + Target: s.Target, + Connected: s.Connected, + Server: s.Server, + Tools: s.Tools, + Err: s.Err, + }) + } + return out +} + +func (w *mcpWiring) close() { + if w == nil { + return + } + _ = w.mgr.Close() +} + +// caller is the tool.MCPCaller the executor gets, or nil when MCP is off. +func (w *mcpWiring) caller() *mcp.Manager { + if w == nil { + return nil + } + return w.mgr +} diff --git a/cmd/mavend/mcp_test.go b/cmd/mavend/mcp_test.go new file mode 100644 index 0000000..6339524 --- /dev/null +++ b/cmd/mavend/mcp_test.go @@ -0,0 +1,77 @@ +package main + +import ( + "context" + "strings" + "testing" + + "github.com/kami/maven/internal/config" +) + +func TestWireMCPOffWhenUnconfigured(t *testing.T) { + st := newTestStore(t) + for name, cfg := range map[string]*config.Config{ + "no block": {}, + "nothing enabled": {MCP: &config.MCPConfig{Servers: []config.MCPServerConfig{ + {Name: "vikunja", URL: "http://192.168.1.104:9100/mcp"}, + }}}, + } { + t.Run(name, func(t *testing.T) { + if w := wireMCP(cfg, st); w != nil { + t.Fatal("MCP must be off unless a server is configured AND enabled") + } + }) + } + // nil wiring must be safe to use everywhere it is reachable. + var w *mcpWiring + w.close() + w.propose(context.Background()) + if w.status() != nil || w.caller() != nil { + t.Fatal("a nil wiring must report nothing") + } +} + +// An unreachable server must not stop the daemon, must be reported as down, and +// must propose nothing. +func TestWireMCPUnreachableServerIsNotFatal(t *testing.T) { + st := newTestStore(t) + w := wireMCP(&config.Config{MCP: &config.MCPConfig{Servers: []config.MCPServerConfig{{ + Name: "dead", Command: "/nonexistent/mcp-server", Enabled: true, + }}}}, st) + if w == nil { + t.Fatal("a configured server should still wire") + } + defer w.close() + st2 := w.status() + if len(st2) != 1 || st2[0].Connected || st2[0].Err == "" { + t.Fatalf("status = %+v", st2) + } + tools, err := st.ListTools(context.Background(), "") + if err != nil { + t.Fatal(err) + } + if len(tools) != 0 { + t.Fatalf("a server that never answered must propose nothing, got %+v", tools) + } +} + +// A url server whose address is private is refused by webfetch unless that +// server sets allow_private. This is the guard the whole MCP path rides on, so +// it is asserted here too, at the wiring level. +func TestWireMCPPrivateURLRefusedWithoutAllowPrivate(t *testing.T) { + st := newTestStore(t) + w := wireMCP(&config.Config{MCP: &config.MCPConfig{Servers: []config.MCPServerConfig{{ + Name: "lan", URL: "http://127.0.0.1:9100/mcp", Enabled: true, + }}}}, st) + if w == nil { + t.Fatal("should wire") + } + defer w.close() + s := w.status()[0] + if s.Connected { + t.Fatal("a loopback server must not connect without allow_private") + } + if !strings.Contains(s.Err, "private address") { + t.Fatalf("err = %q, want the private-address refusal", s.Err) + } +} diff --git a/cmd/mavend/tick.go b/cmd/mavend/tick.go index 3932173..746cbe7 100644 --- a/cmd/mavend/tick.go +++ b/cmd/mavend/tick.go @@ -941,6 +941,7 @@ type daemonAPI struct { getMorningStatus func(ctx context.Context) []ipc.MorningRoutineStatus getDayPlan func(ctx context.Context) ipc.DayPlan chatFn func(ctx context.Context, text string) string + getMCPServers func() []ipc.MCPServerStatus } func (d *daemonAPI) Chat(ctx context.Context, text string) (string, error) { @@ -950,6 +951,16 @@ func (d *daemonAPI) Chat(ctx context.Context, text string) (string, error) { return d.chatFn(ctx, text), nil } +// MCPServers — the configured MCP servers and their health (Vikunja #251). +// Empty, not an error, when the mcp block is absent: "not configured" is the +// default state and the web surface renders it as such. +func (d *daemonAPI) MCPServers(ctx context.Context) ([]ipc.MCPServerStatus, error) { + if d.getMCPServers == nil { + return nil, nil + } + return d.getMCPServers(), nil +} + func (d *daemonAPI) TickTrace(ctx context.Context) (ipc.TickTrace, error) { trace := d.getTrace() if trace == nil { diff --git a/cmd/mavend/voicewire.go b/cmd/mavend/voicewire.go index 06fa81a..77e787e 100644 --- a/cmd/mavend/voicewire.go +++ b/cmd/mavend/voicewire.go @@ -40,6 +40,10 @@ type voiceWiring struct { // mavsttd / mavttsd don't keep a stale conn into a restarting daemon. sttClient *worker.Client ttsClient *worker.Client + // mcp — the MCP client, nil unless the `mcp` block configures an enabled + // server (Vikunja #251). Its tools land in the same allowlist as every + // other act, so nothing else here has to know about it. + mcp *mcpWiring } // close releases the listener + worker conns. Safe to call on nil (when @@ -60,6 +64,7 @@ func (w *voiceWiring) close() { if w.ttsClient != nil { _ = w.ttsClient.Close() } + w.mcp.close() } // wireVoice builds the audio path from cfg + a CoreAPI + a router. Returns @@ -131,6 +136,14 @@ func wireVoice(cfg *config.Config, coreAPI ipc.CoreAPI, phr phraser.Phraser, mem // daemon restart. seedTools(coreAPI, cfg.Voice.Tools) exec := tool.NewExecutor(coreAPI, time.Duration(cfg.Voice.ToolTimeout)) + // MCP servers (Vikunja #251): discovery PROPOSES tools into the same + // allowlist, so an MCP tool is enabled by hand on /tools like any other and + // runs through the same confirm turn. Off unless the `mcp` block configures + // an enabled server. + w.mcp = wireMCP(cfg, dataStore) + if w.mcp != nil { + exec = exec.WithMCP(w.mcp.caller()) + } matcher := tool.NewMatcher(coreAPI) // ----- weather provider (Open-Meteo when configured, Stub otherwise) ----- diff --git a/cmd/mavweb/handlers_test.go b/cmd/mavweb/handlers_test.go index 6eef3fd..162cc9b 100644 --- a/cmd/mavweb/handlers_test.go +++ b/cmd/mavweb/handlers_test.go @@ -67,6 +67,14 @@ type fakeCore struct { // for handleChatAPI tests chatText string chatErr error + + // for the MCP section of /tools + mcpServers []ipc.MCPServerStatus + mcpErr error +} + +func (f *fakeCore) MCPServers(context.Context) ([]ipc.MCPServerStatus, error) { + return f.mcpServers, f.mcpErr } func (f *fakeCore) Chat(_ context.Context, text string) (string, error) { @@ -1118,3 +1126,49 @@ func TestHandleChatAPI_FailOpenByDefault(t *testing.T) { t.Errorf("core.Chat text = %q, want %q", core.chatText, "привет") } } + +// The MCP section renders the configured servers, and a proposal that already +// knows its cmd prefills the enable form so the argv is not retyped by hand. +func TestHandleTools_GET_MCPSection(t *testing.T) { + core := &fakeCore{ + proposed: []ipc.Tool{{ + Name: "vikunja_list_tasks", Scope: "mcp:vikunja", + Cmd: []string{"mcp", "vikunja", "list_tasks"}, Destructive: true, + Utterance: "mcp vikunja/list_tasks: List tasks in a project.", + }}, + mcpServers: []ipc.MCPServerStatus{ + {Name: "vikunja", Transport: "http", Target: "http://192.168.1.104:9100/mcp", Connected: true, Server: "vikunja 0.1.0", Tools: 4}, + {Name: "files", Transport: "stdio", Target: "mcp-server-fs /srv", Err: "start: no such file"}, + }, + } + rr := httptest.NewRecorder() + handleTools(rr, httptest.NewRequest(http.MethodGet, "/tools", nil), core, nil, false) + if rr.Code != http.StatusOK { + t.Fatalf("status = %d", rr.Code) + } + body := rr.Body.String() + for _, want := range []string{ + "MCP servers", "vikunja", "192.168.1.104:9100/mcp", "vikunja 0.1.0", + "files", "no such file", + `value="mcp vikunja list_tasks"`, // the enable form is prefilled + "checked", // and pre-marked destructive (no readOnlyHint) + } { + if !strings.Contains(body, want) { + t.Errorf("missing %q in /tools output", want) + } + } +} + +// MCP off (or an older core that does not know the method) renders the section +// empty instead of breaking the page. +func TestHandleTools_GET_MCPUnavailable(t *testing.T) { + core := &fakeCore{mcpErr: ipc.ErrNotImplemented} + rr := httptest.NewRecorder() + handleTools(rr, httptest.NewRequest(http.MethodGet, "/tools", nil), core, nil, false) + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rr.Code) + } + if !strings.Contains(rr.Body.String(), "no MCP servers configured") { + t.Error("expected the empty-state copy") + } +} diff --git a/cmd/mavweb/main.go b/cmd/mavweb/main.go index 4491084..29be4de 100644 --- a/cmd/mavweb/main.go +++ b/cmd/mavweb/main.go @@ -692,7 +692,7 @@ const toolsHTML = `{{template "shellTop" "tools"}} {{if .Msg}}
{{.Msg}}
{{end}}

proposed {{len .Proposed}}

-{{if .Proposed}}

maven drafted these from acts she couldn't run. Fill the command (argv, space-separated) and enable.

+{{if .Proposed}}

maven drafted these from acts she couldn't run. Fill the command (argv, space-separated) and enable. A row in an mcp: scope came from an MCP server and already knows what it calls — check the command, then enable.

{{range .Proposed}} @@ -700,8 +700,8 @@ const toolsHTML = `{{template "shellTop" "tools"}} - - + + @@ -730,6 +730,19 @@ const toolsHTML = `{{template "shellTop" "tools"}}
enable proposed tools above, or ask maven to configure one
{{end}} +
+

MCP servers {{len .MCP}}

+{{if .MCP}}

servers she connects OUT to. Their tools appear above as proposals — a configured server is a place she may look, not a capability she has. A stdio target is a process on this box; an http one on a loopback or LAN address is inside the network, so treat its tools accordingly.

+
namescopefrom utteranceenable as
{{.Name}}{{.Scope}}{{.Utterance}}
+{{range .MCP}} + +{{end}}
nametransporttargetstatetools
{{.Name}}{{.Transport}}{{.Target}}{{if .Connected}}connected{{if .Server}} — {{.Server}}{{end}}{{else}}down{{if .Err}} — {{.Err}}{{end}}{{end}}{{.Tools}}
+{{else}}
+ +
no MCP servers configured
+
add an mcp.servers block to mavend.json to let her use an external tool server
+
{{end}} +
{{template "shellBottom"}}` // routinesHTML — proposed routine review surface. One row per thing maven @@ -1320,12 +1333,20 @@ func handleTools(w http.ResponseWriter, r *http.Request, core ipc.CoreAPI, sessi http.Error(w, "core read failed", http.StatusBadGateway) return } + // MCP is off by default and an older core may not know the method at all, + // so a failure here renders an empty section rather than breaking the page. + servers, err := core.MCPServers(ctx) + if err != nil { + log.Printf("tools: mcp servers: %v", err) + servers = nil + } w.Header().Set("Content-Type", "text/html; charset=utf-8") if err := toolsTmpl.Execute(w, struct { Msg string Proposed []ipc.Tool Enabled []ipc.Tool - }{msg, proposed, enabled}); err != nil { + MCP []ipc.MCPServerStatus + }{msg, proposed, enabled, servers}); err != nil { log.Printf("tools render: %v", err) } } diff --git a/deploy/mavend.json b/deploy/mavend.json index ed3b9bc..4551e9f 100644 --- a/deploy/mavend.json +++ b/deploy/mavend.json @@ -31,6 +31,20 @@ "cooldown": "24h" }, + "mcp": { + "timeout": "15s", + "servers": [ + { + "name": "vikunja", + "url": "http://192.168.1.104:9100/mcp", + "allow_private": true, + "allow_tools": ["list_projects", "list_tasks", "get_task_details", "create_task"], + "max_tools": 6, + "enabled": false + } + ] + }, + "nexus": { "url": "http://nexus:9740" }, "praxis": { "url": "http://praxis:8989" }, "hexis": { "url": "http://hexis:9741" }, diff --git a/internal/ipc/api.go b/internal/ipc/api.go index a9a0be7..6cd716a 100644 --- a/internal/ipc/api.go +++ b/internal/ipc/api.go @@ -319,6 +319,19 @@ type Tool struct { Updated time.Time `json:"updated"` } +// MCPServerStatus — one configured MCP server, as the web surface sees it. +// Target is the command or url; Tools is how many tools discovery kept after +// allow_tools / max_tools, not how many the server offers. +type MCPServerStatus struct { + Name string `json:"name"` + Transport string `json:"transport"` // "stdio" (a local subprocess) or "http" + Target string `json:"target"` + Connected bool `json:"connected"` + Server string `json:"server,omitempty"` // the server's own name + version + Tools int `json:"tools"` + Err string `json:"err,omitempty"` +} + // chatReq / chatResp — text chat round-trip for the IPC Chat method. type chatReq struct { Text string `json:"text"` @@ -457,6 +470,14 @@ type CoreAPI interface { // TickTrace. MorningStatus(ctx context.Context) ([]MorningRoutineStatus, error) + // MCPServers reports the configured MCP servers and their health + // (Vikunja #251). Read-only introspection for /tools — there is no + // "call this tool" method on purpose: an MCP tool runs through the same + // allowlist, confirm turn and act path as any other tool, and a second + // mutation path would be a second thing to get wrong. Empty when the + // mcp config block is absent, which is the default. + MCPServers(ctx context.Context) ([]MCPServerStatus, error) + // DayPlan returns today's ordered plan — calendar events, pending // reminders and any morning checklist still outstanding (see // internal/morning.BuildPlan) — plus the spoken RU rendering of it. diff --git a/internal/ipc/client.go b/internal/ipc/client.go index 898e1c5..b76378d 100644 --- a/internal/ipc/client.go +++ b/internal/ipc/client.go @@ -72,6 +72,7 @@ var readOnlyMethods = map[Method]bool{ MethodListTasks: true, MethodTickTrace: true, MethodMorningStatus: true, + MethodMCPServers: true, MethodDayPlan: true, } @@ -505,6 +506,14 @@ func (c *Client) TickTrace(ctx context.Context) (TickTrace, error) { return t, nil } +func (c *Client) MCPServers(ctx context.Context) ([]MCPServerStatus, error) { + var s []MCPServerStatus + if err := c.call(ctx, MethodMCPServers, nil, &s); err != nil { + return nil, err + } + return s, nil +} + func (c *Client) MorningStatus(ctx context.Context) ([]MorningRoutineStatus, error) { var s []MorningRoutineStatus if err := c.call(ctx, MethodMorningStatus, nil, &s); err != nil { diff --git a/internal/ipc/server.go b/internal/ipc/server.go index de072fe..f31ce28 100644 --- a/internal/ipc/server.go +++ b/internal/ipc/server.go @@ -211,6 +211,10 @@ func (a *storeAPI) MorningStatus(ctx context.Context) ([]MorningRoutineStatus, e return nil, errors.New("store: morning status not available via direct store API") } +func (a *storeAPI) MCPServers(ctx context.Context) ([]MCPServerStatus, error) { + return nil, nil // no manager behind a bare store: nothing configured +} + func (a *storeAPI) DayPlan(ctx context.Context) (DayPlan, error) { return DayPlan{}, errors.New("store: day plan not available via direct store API") } @@ -832,6 +836,16 @@ var methodTable = map[Method]handlerFunc{ MethodMorningStatus: withoutParams(func(ctx context.Context, api CoreAPI) ([]MorningRoutineStatus, error) { return api.MorningStatus(ctx) }), + MethodMCPServers: withoutParams(func(ctx context.Context, api CoreAPI) ([]MCPServerStatus, error) { + out, err := api.MCPServers(ctx) + if err != nil { + return nil, err + } + if out == nil { + out = []MCPServerStatus{} + } + return out, nil + }), } // dispatch unmarshals params for req.Method and calls the matching CoreAPI diff --git a/internal/ipc/unimplemented.go b/internal/ipc/unimplemented.go index cd0ed11..8c289cb 100644 --- a/internal/ipc/unimplemented.go +++ b/internal/ipc/unimplemented.go @@ -122,6 +122,9 @@ func (UnimplementedCoreAPI) TickTrace(ctx context.Context) (TickTrace, error) { func (UnimplementedCoreAPI) MorningStatus(ctx context.Context) ([]MorningRoutineStatus, error) { return nil, ErrNotImplemented } +func (UnimplementedCoreAPI) MCPServers(ctx context.Context) ([]MCPServerStatus, error) { + return nil, ErrNotImplemented +} func (UnimplementedCoreAPI) DayPlan(ctx context.Context) (DayPlan, error) { return DayPlan{}, ErrNotImplemented } diff --git a/internal/ipc/wire.go b/internal/ipc/wire.go index 151eebf..8f1183d 100644 --- a/internal/ipc/wire.go +++ b/internal/ipc/wire.go @@ -45,6 +45,7 @@ const ( MethodRevertFact Method = "revert_fact" MethodTickTrace Method = "tick_trace" MethodMorningStatus Method = "morning_status" + MethodMCPServers Method = "mcp_servers" MethodDayPlan Method = "day_plan" MethodChat Method = "chat" MethodCaptureTask Method = "capture_task" diff --git a/internal/mcp/manager.go b/internal/mcp/manager.go index edeb017..cec97e9 100644 --- a/internal/mcp/manager.go +++ b/internal/mcp/manager.go @@ -2,10 +2,12 @@ package mcp import ( "context" + "encoding/json" "errors" "fmt" "log" "sort" + "strconv" "strings" "sync" "time" @@ -429,3 +431,93 @@ func (m *Manager) Close() error { } return nil } + +// ErrNeedsArgs — the tool requires arguments that a voice verb cannot supply. +var ErrNeedsArgs = errors.New("mcp: tool needs named arguments") + +// CallPositional is the voice path's way in. The router gives an act a verb and +// a tail of positional words; an MCP tool wants a named-argument object. There +// is no general mapping between those two, and inventing one is exactly the +// improvisation this codebase refuses, so the rule is deliberately narrow: +// +// - a tool with no required properties runs with no arguments (a spare tail +// is ignored — "покажи проекты пожалуйста" should still list projects); +// - a READ-ONLY tool with exactly one required property, of type string or +// integer/number, gets the tail bound to it; +// - anything else is refused with ErrNeedsArgs. Such a tool is still callable +// with explicit arguments from the authed surface, where a human types +// them. +// +// The refusal is the point, and the read-only condition on it was learned the +// hard way while testing against the Vikunja server: `update_task` requires +// only `task_id` and takes every other field as optional, so calling it with +// one guessed argument and no others BLANKED the fields it did not receive. A +// mutating tool therefore never gets a guessed argument — the one thing a +// partially-filled write can do is destroy what it did not mention. A mutating +// tool with nothing required is still fine: nothing was guessed, and it still +// goes through the confirm turn. +func (m *Manager) CallPositional(ctx context.Context, server, tool string, args []string) (string, error) { + m.mu.Lock() + c := m.conns[server] + var schema json.RawMessage + found, readOnly := false, false + if c != nil { + for _, t := range c.tools { + if t.Name == tool { + schema, readOnly, found = t.InputSchema, t.ReadOnly, true + break + } + } + } + m.mu.Unlock() + if !found { + return "", fmt.Errorf("mcp: %s offers no tool %q", server, tool) + } + named, err := bindPositional(schema, args, readOnly) + if err != nil { + return "", err + } + return m.Call(ctx, server, tool, named) +} + +// bindPositional implements the rule documented on CallPositional. +func bindPositional(schema json.RawMessage, args []string, readOnly bool) (map[string]any, error) { + var s struct { + Required []string `json:"required"` + Properties map[string]struct { + Type string `json:"type"` + } `json:"properties"` + } + if len(schema) > 0 { + if err := json.Unmarshal(schema, &s); err != nil { + return nil, fmt.Errorf("mcp: unreadable input schema: %w", err) + } + } + switch len(s.Required) { + case 0: + return map[string]any{}, nil + case 1: + name := s.Required[0] + if !readOnly { + return nil, fmt.Errorf("%w: %q, and a tool that writes never gets a guessed one", ErrNeedsArgs, name) + } + tail := strings.TrimSpace(strings.Join(args, " ")) + if tail == "" { + return nil, fmt.Errorf("%w: %q", ErrNeedsArgs, name) + } + switch s.Properties[name].Type { + case "string", "": + return map[string]any{name: tail}, nil + case "integer", "number": + n, err := strconv.ParseFloat(tail, 64) + if err != nil { + return nil, fmt.Errorf("%w: %q wants a number, got %q", ErrNeedsArgs, name, tail) + } + return map[string]any{name: n}, nil + default: + return nil, fmt.Errorf("%w: %q is a %s", ErrNeedsArgs, name, s.Properties[name].Type) + } + default: + return nil, fmt.Errorf("%w: %s", ErrNeedsArgs, strings.Join(s.Required, ", ")) + } +} diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go index 3f29e6c..0888e25 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -444,3 +444,122 @@ func TestLocalNameAndCmd(t *testing.T) { t.Error("scope") } } + +func TestBindPositional(t *testing.T) { + cases := []struct { + name, schema string + args []string + mutating bool + want map[string]any + wantErr bool + }{ + { + name: "no required runs with nothing", + // A spare tail is fine: "покажи проекты пожалуйста" still lists them. + schema: `{"type":"object","properties":{},"required":[]}`, + args: []string{"пожалуйста"}, + want: map[string]any{}, + }, + { + name: "empty schema", + schema: ``, + want: map[string]any{}, + }, + { + name: "one required string gets the tail", + schema: `{"properties":{"q":{"type":"string"}},"required":["q"]}`, + args: []string{"почему", "небо", "синее"}, + want: map[string]any{"q": "почему небо синее"}, + }, + { + name: "one required string with no tail", + schema: `{"properties":{"q":{"type":"string"}},"required":["q"]}`, + wantErr: true, + }, + { + name: "one required integer parses", + schema: `{"properties":{"task_id":{"type":"integer"}},"required":["task_id"]}`, + args: []string{"251"}, + want: map[string]any{"task_id": float64(251)}, + }, + { + name: "one required integer with words", + schema: `{"properties":{"task_id":{"type":"integer"}},"required":["task_id"]}`, + args: []string{"двести", "пятьдесят", "один"}, + wantErr: true, + }, + { + name: "two required is refused rather than guessed", + schema: `{"properties":{"a":{"type":"string"},"b":{"type":"string"}},"required":["a","b"]}`, + args: []string{"что-то"}, + wantErr: true, + }, + { + name: "one required object is refused", + schema: `{"properties":{"payload":{"type":"object"}},"required":["payload"]}`, + args: []string{"что-то"}, + wantErr: true, + }, + { + // Learned from Vikunja's update_task: required ["task_id"], every + // other field optional, so one guessed argument blanks the rest. + name: "one required on a mutating tool is refused", + schema: `{"properties":{"task_id":{"type":"integer"}},"required":["task_id"]}`, + args: []string{"251"}, + mutating: true, + wantErr: true, + }, + { + // Nothing was guessed, so there is nothing to get wrong. It still + // goes through the confirm turn upstream. + name: "no required on a mutating tool still runs", + schema: `{"properties":{},"required":[]}`, + mutating: true, + want: map[string]any{}, + }, + { + name: "unreadable schema", + schema: `not json`, + wantErr: true, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := bindPositional(json.RawMessage(tc.schema), tc.args, !tc.mutating) + if tc.wantErr { + if err == nil { + t.Fatalf("want an error, got %v", got) + } + return + } + if err != nil { + t.Fatal(err) + } + if fmt.Sprint(got) != fmt.Sprint(tc.want) { + t.Fatalf("got %v want %v", got, tc.want) + } + }) + } +} + +func TestCallPositionalThroughManager(t *testing.T) { + p := &fakePoster{handler: echoServer()} + m, err := NewManager(func(ServerConfig) (Poster, error) { return p, nil }, + []ServerConfig{{Name: "fake", URL: "http://example.test/mcp", Enabled: true}}) + if err != nil { + t.Fatal(err) + } + m.Connect(context.Background()) + defer m.Close() + // echoServer's tools declare no required properties. + out, err := m.CallPositional(context.Background(), "fake", "read_thing", []string{"хвост"}) + if err != nil { + t.Fatalf("call: %v", err) + } + if out != "read_thing:" { + t.Fatalf("out = %q", out) + } + if _, err := m.CallPositional(context.Background(), "fake", "absent", nil); err == nil { + t.Error("an unknown tool must be refused") + } +} diff --git a/internal/store/tools.go b/internal/store/tools.go index 2c86af8..6455fcd 100644 --- a/internal/store/tools.go +++ b/internal/store/tools.go @@ -56,6 +56,45 @@ func (s *Store) ProposeTool(ctx context.Context, name, utterance, scope string, return n > 0, nil } +// ProposeMCPTool is ProposeTool for a tool discovered on an MCP server +// (Vikunja #251): the proposal already knows what it would run, so cmd and +// destructive are written with it and Kami only has to press enable. +// +// It is still a PROPOSAL. Discovery cannot grant a capability — that is the +// whole reason a server can be configured without its tools becoming live. +// Like ProposeTool it never touches an existing row, so re-discovery on every +// restart is idempotent and cannot silently re-arm a tool that was disabled or +// change the cmd of one already enabled. +func (s *Store) ProposeMCPTool(ctx context.Context, name, scope string, cmd []string, destructive bool, utterance string, ts time.Time) (bool, error) { + if len(cmd) == 0 { + return false, ErrToolCmd + } + if scope == "" { + scope = "homelab" + } + raw, err := json.Marshal(cmd) + if err != nil { + return false, fmt.Errorf("propose mcp tool: %w", err) + } + d := 0 + if destructive { + d = 1 + } + res, err := s.db.ExecContext(ctx, ` + INSERT INTO tools (name, scope, cmd, destructive, status, utterance, created_ts, updated_ts) + VALUES (?, ?, ?, ?, 'proposed', ?, ?, ?) + ON CONFLICT(name) DO NOTHING`, + name, scope, string(raw), d, utterance, ts.UnixMilli(), ts.UnixMilli()) + if err != nil { + return false, fmt.Errorf("propose mcp tool: %w", err) + } + n, err := res.RowsAffected() + if err != nil { + return false, fmt.Errorf("propose mcp tool: rows affected: %w", err) + } + return n > 0, nil +} + // EnableTool fills cmd + destructive and flips status to 'enabled'. This is the // human "enable" act (the authed surface calls it); it upserts so enabling a // name that was never proposed still works. An empty cmd is refused — an diff --git a/internal/store/tools_test.go b/internal/store/tools_test.go index 1111e6b..0ee973e 100644 --- a/internal/store/tools_test.go +++ b/internal/store/tools_test.go @@ -2,6 +2,7 @@ package store import ( "context" + "errors" "testing" "time" ) @@ -60,3 +61,64 @@ func TestToolLifecycle(t *testing.T) { t.Fatalf("disable absent must be no-op: %v", err) } } + +// A discovered MCP tool arrives as a proposal that already knows its cmd, so +// enabling it is one click rather than one retyped argv. +func TestProposeMCPTool(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Now() + cmd := []string{"mcp", "vikunja", "list_tasks"} + + fresh, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, false, "mcp vikunja/list_tasks: List tasks", now) + if err != nil { + t.Fatal(err) + } + if !fresh { + t.Fatal("first proposal should be new") + } + got, err := s.LookupTool(ctx, "vikunja_list_tasks") + if err != nil { + t.Fatal(err) + } + if got.Status != "proposed" { + t.Fatalf("status = %q — discovery must never enable", got.Status) + } + if len(got.Cmd) != 3 || got.Cmd[0] != "mcp" || got.Cmd[2] != "list_tasks" { + t.Fatalf("cmd = %v", got.Cmd) + } + if got.Scope != "mcp:vikunja" || got.Utterance == "" { + t.Fatalf("provenance lost: %+v", got) + } + + // Re-discovery on the next boot is idempotent. + fresh, err = s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", cmd, true, "changed", now) + if err != nil { + t.Fatal(err) + } + if fresh { + t.Error("re-proposing an existing row must report nothing new") + } + + // And it must not re-arm or rewrite a row a human already acted on. + if err := s.EnableTool(ctx, "vikunja_list_tasks", cmd, false, "mcp:vikunja", now); err != nil { + t.Fatal(err) + } + if _, err := s.ProposeMCPTool(ctx, "vikunja_list_tasks", "mcp:vikunja", []string{"mcp", "vikunja", "delete_task"}, true, "x", now); err != nil { + t.Fatal(err) + } + got, err = s.LookupTool(ctx, "vikunja_list_tasks") + if err != nil { + t.Fatal(err) + } + if got.Status != "enabled" || got.Cmd[2] != "list_tasks" || got.Destructive { + t.Fatalf("an enabled row was modified by discovery: %+v", got) + } +} + +func TestProposeMCPToolNeedsCmd(t *testing.T) { + s := newTestStore(t) + if _, err := s.ProposeMCPTool(context.Background(), "x", "mcp:y", nil, false, "", time.Now()); !errors.Is(err, ErrToolCmd) { + t.Fatalf("err = %v, want ErrToolCmd", err) + } +} diff --git a/internal/tool/tool.go b/internal/tool/tool.go index abe0977..4d16c6f 100644 --- a/internal/tool/tool.go +++ b/internal/tool/tool.go @@ -13,6 +13,11 @@ // - Args are passed as argv, NEVER through a shell. STT text lands as // positional arguments to Cmd; there is no `sh -c`, so "restart nginx; // rm -rf" can't inject — the tail is one argv element to the named binary. +// - An enabled row whose cmd is ["mcp", "", ""] is a call to a +// configured MCP server instead of a process (Vikunja #251). It goes +// through every rule above unchanged — enabled, and confirmed if it +// mutates — because the store is still the allowlist; only the dispatch at +// the bottom of Exec differs. // - Destructive tools don't run on first hearing: Exec returns ErrNeedsConfirm // and the handler runs a confirm turn ("выполнить X? да/нет"); only a // confirmed re-Exec runs them. A gate assumes a fully-formed action, which @@ -31,6 +36,7 @@ import ( "time" "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/mcp" "github.com/kami/maven/internal/router" ) @@ -50,12 +56,21 @@ var ( ErrNeedsConfirm = errors.New("destructive tool needs confirmation") ) +// MCPCaller is the seam for an act that is an MCP tool call rather than a +// process (Vikunja #251). internal/mcp.Manager satisfies it via CallPositional. +// nil ⇒ MCP is not configured, and an MCP row refuses to run rather than +// silently doing nothing. +type MCPCaller interface { + CallPositional(ctx context.Context, server, tool string, args []string) (string, error) +} + // Executor runs enabled tools. run is the exec seam (default: real process); // tests swap it. timeout bounds each invocation. type Executor struct { api API timeout time.Duration run func(ctx context.Context, argv []string) (string, error) + mcp MCPCaller } // NewExecutor builds the executor. timeout<=0 defaults to 30s. @@ -66,6 +81,13 @@ func NewExecutor(api API, timeout time.Duration) *Executor { return &Executor{api: api, timeout: timeout, run: runProcess} } +// WithMCP attaches the MCP caller. Called once at wiring time when the mcp +// config block is present; without it, a row whose cmd is ["mcp", …] refuses. +func (e *Executor) WithMCP(m MCPCaller) *Executor { + e.mcp = m + return e +} + // Exec looks up name in the store and runs Cmd+args as argv (no shell). // confirmed=true is the second turn of a destructive act (the user said "да"); // it bypasses the ErrNeedsConfirm gate. Non-enabled ⇒ ErrNotEnabled; a @@ -84,6 +106,17 @@ func (e *Executor) Exec(ctx context.Context, name string, args []string, confirm if t.Destructive && !confirmed { return "", ErrNeedsConfirm } + // An MCP row is a call to a configured server, not a process. Everything + // above still applied: it had to be enabled, and a mutating one had to be + // confirmed. Only the dispatch differs. + if server, remote, ok := mcp.ParseCmd(t.Cmd); ok { + if e.mcp == nil { + return "", ErrNotEnabled + } + ctx, cancel := context.WithTimeout(ctx, e.timeout) + defer cancel() + return e.mcp.CallPositional(ctx, server, remote, args) + } argv := append(append([]string(nil), t.Cmd...), args...) if len(argv) == 0 { return "", ErrNotEnabled diff --git a/internal/tool/tool_test.go b/internal/tool/tool_test.go index a28b4bb..f0c5dcd 100644 --- a/internal/tool/tool_test.go +++ b/internal/tool/tool_test.go @@ -85,3 +85,103 @@ func TestExec(t *testing.T) { t.Fatal("proposed tool must not match (not enabled)") } } + +// fakeMCP records what the executor asked it to call. +type fakeMCP struct { + server, tool string + args []string + out string + err error + calls int +} + +func (f *fakeMCP) CallPositional(_ context.Context, server, tool string, args []string) (string, error) { + f.calls++ + f.server, f.tool, f.args = server, tool, args + return f.out, f.err +} + +// An MCP row dispatches to the caller instead of a process, and the process +// seam is never touched. +func TestExecMCPRowDispatchesToMCP(t *testing.T) { + api := fakeAPI{tools: map[string]ipc.Tool{ + "vikunja_list_tasks": { + Name: "vikunja_list_tasks", Status: "enabled", Scope: "mcp:vikunja", + Cmd: []string{"mcp", "vikunja", "list_tasks"}, + }, + }} + m := &fakeMCP{out: "две задачи"} + ran := false + e := NewExecutor(api, time.Second).WithMCP(m) + e.run = func(context.Context, []string) (string, error) { ran = true; return "", nil } + + out, err := e.Exec(context.Background(), "vikunja_list_tasks", []string{"мавен"}, false) + if err != nil { + t.Fatalf("exec: %v", err) + } + if out != "две задачи" { + t.Fatalf("out = %q", out) + } + if ran { + t.Fatal("an MCP row must not be executed as a process") + } + if m.server != "vikunja" || m.tool != "list_tasks" || len(m.args) != 1 || m.args[0] != "мавен" { + t.Fatalf("dispatched wrong: %+v", m) + } +} + +// The allowlist rules still apply to an MCP row: destructive means a confirm +// turn first, and nothing is called until the second turn. +func TestExecMCPRowStillNeedsConfirm(t *testing.T) { + api := fakeAPI{tools: map[string]ipc.Tool{ + "vikunja_delete_task": { + Name: "vikunja_delete_task", Status: "enabled", Destructive: true, + Cmd: []string{"mcp", "vikunja", "delete_task"}, + }, + }} + m := &fakeMCP{out: "удалила"} + e := NewExecutor(api, time.Second).WithMCP(m) + if _, err := e.Exec(context.Background(), "vikunja_delete_task", nil, false); !errors.Is(err, ErrNeedsConfirm) { + t.Fatalf("err = %v, want ErrNeedsConfirm", err) + } + if m.calls != 0 { + t.Fatal("a destructive MCP tool must not reach the server before confirmation") + } + if _, err := e.Exec(context.Background(), "vikunja_delete_task", nil, true); err != nil { + t.Fatalf("confirmed exec: %v", err) + } + if m.calls != 1 { + t.Fatalf("calls = %d", m.calls) + } +} + +// A proposed MCP row does not run, exactly like a proposed shell tool. +func TestExecMCPRowNotEnabled(t *testing.T) { + api := fakeAPI{tools: map[string]ipc.Tool{ + "vikunja_list_tasks": {Name: "vikunja_list_tasks", Status: "proposed", Cmd: []string{"mcp", "vikunja", "list_tasks"}}, + }} + m := &fakeMCP{} + e := NewExecutor(api, time.Second).WithMCP(m) + if _, err := e.Exec(context.Background(), "vikunja_list_tasks", nil, false); !errors.Is(err, ErrNotEnabled) { + t.Fatalf("err = %v", err) + } + if m.calls != 0 { + t.Fatal("a proposal must not call anything") + } +} + +// With MCP unconfigured, an MCP row refuses rather than trying to exec "mcp". +func TestExecMCPRowWithoutCallerRefuses(t *testing.T) { + api := fakeAPI{tools: map[string]ipc.Tool{ + "vikunja_list_tasks": {Name: "vikunja_list_tasks", Status: "enabled", Cmd: []string{"mcp", "vikunja", "list_tasks"}}, + }} + ran := false + e := NewExecutor(api, time.Second) + e.run = func(context.Context, []string) (string, error) { ran = true; return "", nil } + if _, err := e.Exec(context.Background(), "vikunja_list_tasks", nil, false); !errors.Is(err, ErrNotEnabled) { + t.Fatalf("err = %v, want ErrNotEnabled", err) + } + if ran { + t.Fatal(`"mcp" must never be run as a binary`) + } +} -- 2.52.0