Expose discovered MCP tools through the act allowlist (#251) #71

Closed
claude wants to merge 1 commits from overnight/mcp-tools into overnight/mcp-client
20 changed files with 863 additions and 4 deletions
+7
View File
@@ -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 != "" {
+15
View File
@@ -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()
+154
View File
@@ -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
}
+77
View File
@@ -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)
}
}
+11
View File
@@ -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 {
+13
View File
@@ -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) -----
+54
View File
@@ -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")
}
}
+25 -4
View File
@@ -692,7 +692,7 @@ const toolsHTML = `{{template "shellTop" "tools"}}
{{if .Msg}}<div class="msg msg-ok">{{.Msg}}</div>{{end}}
<section class=card>
<h2 class=card-title>proposed <span class=badge>{{len .Proposed}}</span></h2>
{{if .Proposed}}<p class=hint>maven drafted these from acts she couldn't run. Fill the command (argv, space-separated) and enable.</p>
{{if .Proposed}}<p class=hint>maven drafted these from acts she couldn't run. Fill the command (argv, space-separated) and enable. A row in an <code>mcp:</code> scope came from an MCP server and already knows what it calls — check the command, then enable.</p>
<div class=scroll><table><tr><th>name</th><th>scope</th><th>from utterance</th><th>enable as</th></tr>
{{range .Proposed}}<tr>
<td><code>{{.Name}}</code></td><td><span class=badge>{{.Scope}}</span></td><td>{{.Utterance}}</td>
@@ -700,8 +700,8 @@ const toolsHTML = `{{template "shellTop" "tools"}}
<input type=hidden name=name value="{{.Name}}">
<input type=hidden name=scope value="{{.Scope}}">
<input type=hidden name=action value=enable>
<input type=text name=cmd class=input-wide placeholder="systemctl restart" required>
<label><input type=checkbox name=destructive> destructive</label>
<input type=text name=cmd class=input-wide placeholder="systemctl restart" value="{{join .Cmd " "}}" required>
<label><input type=checkbox name=destructive {{if .Destructive}}checked{{end}}> destructive</label>
<button class=btn>enable</button></form>
<form method=post action=/tools class=inline-form>
<input type=hidden name=name value="{{.Name}}">
@@ -730,6 +730,19 @@ const toolsHTML = `{{template "shellTop" "tools"}}
<div class=hint>enable proposed tools above, or ask maven to configure one</div>
</div>{{end}}
</section>
<section class=card>
<h2 class=card-title>MCP servers <span class=badge>{{len .MCP}}</span></h2>
{{if .MCP}}<p class=hint>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 <code>stdio</code> target is a process on this box; an <code>http</code> one on a loopback or LAN address is inside the network, so treat its tools accordingly.</p>
<div class=scroll><table><tr><th>name</th><th>transport</th><th>target</th><th>state</th><th>tools</th></tr>
{{range .MCP}}<tr><td><code>{{.Name}}</code></td><td><span class=badge>{{.Transport}}</span></td><td><code>{{.Target}}</code></td>
<td>{{if .Connected}}connected{{if .Server}}{{.Server}}{{end}}{{else}}<span class=red>down</span>{{if .Err}}{{.Err}}{{end}}{{end}}</td>
<td>{{.Tools}}</td></tr>{{end}}</table></div>
{{else}}<div class=empty>
<svg class=icon width="20" height="20"><use href="/ethos-icons.svg#i-settings"/></svg>
<div>no MCP servers configured</div>
<div class=hint>add an <code>mcp.servers</code> block to mavend.json to let her use an external tool server</div>
</div>{{end}}
</section>
{{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)
}
}
+14
View File
@@ -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" },
+21
View File
@@ -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.
+9
View File
@@ -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 {
+14
View File
@@ -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
+3
View File
@@ -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
}
+1
View File
@@ -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"
+92
View File
@@ -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, ", "))
}
}
+119
View File
@@ -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:<nil>" {
t.Fatalf("out = %q", out)
}
if _, err := m.CallPositional(context.Background(), "fake", "absent", nil); err == nil {
t.Error("an unknown tool must be refused")
}
}
+39
View File
@@ -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
+62
View File
@@ -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)
}
}
+33
View File
@@ -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", "<server>", "<tool>"] 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
+100
View File
@@ -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`)
}
}