From 5e0417306b1eb9159268eb940c285c427509f4e9 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:11:39 +0400 Subject: [PATCH] mcp: guard the connection, not just the first dial Refresh called alive() with the manager lock held, so a slow health check blocked every other server. It now snapshots the candidates and asks outside the lock. A server that cannot be dialled was retried every minute forever, which for a misconfigured stdio block means re-exec'ing a process 1440 times a day. Dials now back off from one minute to thirty. An allow_private fetcher followed redirects. A LAN MCP endpoint could answer a POST with a redirect to 169.254.169.254 and the guard would go there, because allow_private is what turns the address check off. Redirects are refused outright on that door. The tool catalogue was trimmed by taking the first max_tools entries of whatever order the server sent, so the server chose which of its tools Maven proposed. Over the cap without allow_tools now contributes nothing: refusing is honest, silently keeping the server's pick is not. Descriptions are server-written text that lands in the router prompt and on /tools, so they are capped too. A server block with enabled false was skipped by validation, so a typo in a block written dark surfaced only on the day it was switched on. All blocks are shape-checked now. Configured static headers carry the bearer token a real remote server needs, and host_interval bounds how fast one endpoint is polled. Found in review of #70. --- internal/config/config.go | 57 ++++++++++-- internal/config/mcp_test.go | 47 +++++++++- internal/mcp/manager.go | 173 +++++++++++++++++++++++++++++------ internal/mcp/mcp_test.go | 94 +++++++++++++++++-- internal/mcp/webfetchdoor.go | 10 ++ 5 files changed, 337 insertions(+), 44 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 0124e1a..84af890 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -273,8 +273,20 @@ type MCPConfig struct { // MaxBytes — cap on one JSON-RPC response. 0 ⇒ webfetch.DefaultMaxBytes. MaxBytes int64 `json:"max_bytes,omitempty"` + + // HostInterval — minimum spacing between two requests to one MCP server. + // 0 ⇒ DefaultMCPHostInterval (50ms), NOT webfetch's own one-second default. + // That default was sized for a feed poll loop, and this path is in a spoken + // turn: one dial is three requests (initialize, initialized, tools/list), + // so a second of spacing is two seconds of pure sleeping per dial and up to + // another second before every tools/call leaves the box. + HostInterval Duration `json:"host_interval,omitempty"` } +// DefaultMCPHostInterval — see MCPConfig.HostInterval. Enough to stop a +// runaway loop hammering a server, small enough not to be heard. +const DefaultMCPHostInterval = 50 * time.Millisecond + // SmartHomeConfig — the Home Assistant block (Vikunja #256). Dark until // `"enabled": true`, and even then a discovered device is only ever PROPOSED // into the act allowlist: Kami enables it on /tools, behind step-up, exactly as @@ -416,6 +428,13 @@ type MCPServerConfig struct { // Timeout — per-call budget for this server. 0 ⇒ MCPConfig.Timeout. Timeout Duration `json:"timeout,omitempty"` + // Headers — sent verbatim on every request to a url server. This is how a + // bearer token reaches a real remote MCP server: {"Authorization": "Bearer + // ${MCP_TOKEN}"}, with the value in the gitignored env file like the + // telegram credentials. The Vikunja server on homesrv needs none only + // because it is unauthenticated on loopback. + Headers map[string]string `json:"headers,omitempty"` + // Enabled — false (the default) keeps a configured server described but // dark, so a block can be written and reviewed before it is switched on. Enabled bool `json:"enabled,omitempty"` @@ -424,13 +443,31 @@ type MCPServerConfig struct { // MCPServers maps the config blocks onto the mcp package's own type. It lives // here so config validation and daemon wiring cannot drift on the mapping. // Returns nil when nothing is configured or nothing is enabled. +// +// Disabled servers are dropped here, which is why validation does NOT use this +// list — see allMCPServers. func (c *Config) MCPServers() []mcp.ServerConfig { + return c.mcpServers(true) +} + +// allMCPServers is every configured server, enabled or not, for validation. +// +// Validating only the enabled ones meant a block with both command and url, or +// a bare hostname as the url, passed startup validation while it was 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 was the one thing skipped. Enabled +// gates the dialing, not the shape check. +func (c *Config) allMCPServers() []mcp.ServerConfig { + return c.mcpServers(false) +} + +func (c *Config) mcpServers(onlyEnabled bool) []mcp.ServerConfig { if c.MCP == nil { return nil } out := make([]mcp.ServerConfig, 0, len(c.MCP.Servers)) for _, s := range c.MCP.Servers { - if !s.Enabled { + if onlyEnabled && !s.Enabled { continue } timeout := time.Duration(s.Timeout) @@ -447,8 +484,9 @@ func (c *Config) MCPServers() []mcp.ServerConfig { AllowPrivate: s.AllowPrivate, AllowTools: s.AllowTools, MaxTools: s.MaxTools, + Headers: s.Headers, Timeout: timeout, - Enabled: true, + Enabled: s.Enabled, }) } if len(out) == 0 { @@ -1236,11 +1274,18 @@ func (c *Config) applyDefaults() { c.Feeds = nil } - // Same rule for MCP: a block with no server, or none enabled, is the same - // as no block at all. Normalising it to nil keeps "off" in one place. - if c.MCP != nil && len(c.MCPServers()) == 0 { + // Same rule for MCP: a block with no server at all is the same as no block. + // A block whose servers are all disabled is NOT normalised away, because + // validate has to see their shape — a dark block with a typo in it should + // fail at startup, which is the whole reason it can be written before it is + // switched on. wireMCP builds nothing when nothing is enabled, so "off" + // still holds. + if c.MCP != nil && len(c.MCP.Servers) == 0 { c.MCP = nil } + if c.MCP != nil && c.MCP.HostInterval <= 0 { + c.MCP.HostInterval = Duration(DefaultMCPHostInterval) + } // Same rule for the house: a block that is not enabled is the same as no // block at all, so "off" stays in one place. @@ -1363,7 +1408,7 @@ func (c *Config) validate() error { // An MCP block with a typo (no name, both command and url, a bare hostname // as the url) fails here, at startup, rather than at the first turn that // needed the tool. - if err := mcp.Validate(c.MCPServers()); err != nil { + if err := mcp.Validate(c.allMCPServers()); err != nil { return err } // Same for the house: a missing token or a bare hostname fails at startup, diff --git a/internal/config/mcp_test.go b/internal/config/mcp_test.go index 48e05e2..e11de02 100644 --- a/internal/config/mcp_test.go +++ b/internal/config/mcp_test.go @@ -26,14 +26,55 @@ func TestMCPDisabledServerIsOff(t *testing.T) { if err != nil { t.Fatal(err) } - if c.MCP != nil { - t.Errorf("a block with nothing enabled must normalise to nil, got %+v", c.MCP) - } if got := c.MCPServers(); len(got) != 0 { t.Errorf("MCPServers() = %+v", got) } } +// A block with no servers at all is the same as no block. +func TestMCPEmptyBlockNormalisesToNil(t *testing.T) { + c, err := Load(writeConfig(t, `{"mcp":{"servers":[]}}`)) + if err != nil { + t.Fatal(err) + } + if c.MCP != nil { + t.Errorf("mcp = %+v, want nil", c.MCP) + } +} + +// A server that is written but not switched on is still shape-checked. The +// review the config layer can give is the point of writing a block dark, and +// skipping it meant a typo only surfaced on the day it was enabled. +func TestMCPDisabledServerIsStillValidated(t *testing.T) { + cases := map[string]string{ + "both": `{"mcp":{"servers":[{"name":"a","command":"x","url":"http://a.test"}]}}`, + "bare host": `{"mcp":{"servers":[{"name":"a","url":"a.test"}]}}`, + "no name": `{"mcp":{"servers":[{"command":"x"}]}}`, + "duplicates": `{"mcp":{"servers":[{"name":"a","command":"x"},{"name":"a","command":"y"}]}}`, + } + for name, body := range cases { + t.Run(name, func(t *testing.T) { + if _, err := Load(writeConfig(t, body)); err == nil { + t.Fatal("a dark server with a typo must fail at startup") + } + }) + } +} + +// Headers carry a bearer token to a real remote server. +func TestMCPServerHeaders(t *testing.T) { + c, err := Load(writeConfig(t, `{"mcp":{"servers":[ + {"name":"remote","url":"https://mcp.example.test/mcp","enabled":true, + "headers":{"Authorization":"Bearer sekret"}}]}}`)) + if err != nil { + t.Fatal(err) + } + got := c.MCPServers() + if len(got) != 1 || got[0].Headers["Authorization"] != "Bearer sekret" { + t.Fatalf("headers not mapped: %+v", got) + } +} + func TestMCPEnabledServerMapping(t *testing.T) { c, err := Load(writeConfig(t, `{"mcp":{ "timeout":"5s", diff --git a/internal/mcp/manager.go b/internal/mcp/manager.go index cec97e9..d8a7dc4 100644 --- a/internal/mcp/manager.go +++ b/internal/mcp/manager.go @@ -24,12 +24,31 @@ const ( // wrong act. Twelve per server is already generous. DefaultMaxTools = 12 // DefaultReconnectEvery is how long the manager waits before re-dialing a - // server whose connection died. + // server whose connection died. It is the FIRST wait: every consecutive + // failure doubles it, up to MaxReconnectEvery. DefaultReconnectEvery = 30 * time.Second + // MaxReconnectEvery caps the backoff. Without one, a permanently + // misconfigured stdio server is exec'd once a minute forever, which is a + // process spawn per minute in the logs and nothing that ever gets better. + MaxReconnectEvery = 30 * time.Minute + // DefaultMaxDescription bounds one tool description. It is written by a + // server Maven does not control and it lands in two places that cannot + // absorb an arbitrary blob: the resident model's 4096-token context, and a + // table cell on /tools. + DefaultMaxDescription = 400 ) -// ErrNoServer — the named server is not configured or not connected. -var ErrNoServer = errors.New("mcp: no such server") +var ( + // ErrNoServer — the named server is not configured. + ErrNoServer = errors.New("mcp: no such server") + // ErrNotConnected — the server is configured but nothing is dialed. Held + // apart from ErrNoServer so a caller can say "that tool is not connected" + // instead of drafting a proposal for a tool that already exists. + ErrNotConnected = errors.New("mcp: server is not connected") + // ErrToolGone — the server no longer offers this tool. An enabled row can + // outlive the tool it names; this is what the act path sees when it does. + ErrToolGone = errors.New("mcp: server no longer offers this tool") +) // ServerConfig is one configured MCP server. Off unless present. // @@ -61,6 +80,10 @@ type ServerConfig struct { AllowTools []string `json:"allow_tools,omitempty"` // MaxTools caps the contribution (0 ⇒ DefaultMaxTools). MaxTools int `json:"max_tools,omitempty"` + // Headers are sent verbatim on every request to a url server. This is how + // a bearer token reaches a real remote server; the Vikunja one on loopback + // needs none only because it is unauthenticated. + Headers map[string]string `json:"-"` // Timeout bounds one call (0 ⇒ DefaultTimeout). Timeout time.Duration `json:"-"` // Enabled=false keeps a configured server described but dark. @@ -90,6 +113,20 @@ type conn struct { lastErr error lastTry time.Time dialedAt time.Time + fails int // consecutive dial failures, for the backoff +} + +// backoff is how long this connection waits before the next dial attempt: +// DefaultReconnectEvery doubled per consecutive failure, capped. +func (c *conn) backoff() time.Duration { + d := DefaultReconnectEvery + for i := 1; i < c.fails && d < MaxReconnectEvery; i++ { + d *= 2 + } + if d > MaxReconnectEvery { + d = MaxReconnectEvery + } + return d } // NewManager builds a manager for the enabled servers in cfgs. newPoster is @@ -202,7 +239,7 @@ func (m *Manager) dial(ctx context.Context, name string) error { } else { var poster Poster if poster, err = m.newPoster(cfg); err == nil { - tr = newHTTPTransport(poster, cfg.URL) + tr = newHTTPTransport(poster, cfg.URL, cfg.Headers) } } if err != nil { @@ -233,6 +270,7 @@ func (m *Manager) dial(ctx context.Context, name string) error { m.conns[name].client = cl m.conns[name].tools = tools m.conns[name].lastErr = nil + m.conns[name].fails = 0 m.conns[name].dialedAt = time.Now() m.mu.Unlock() log.Printf("mcp: %s connected (%s %s), %d tool(s)", name, cl.Info().Name, cl.Info().Version, len(tools)) @@ -246,12 +284,19 @@ func (m *Manager) fail(name string, err error) { c.lastErr = err c.client = nil c.tools = nil + c.fails++ } } -// filterTools applies AllowTools and MaxTools, and drops nameless entries. -// Sorted first, so the cap is deterministic rather than "whatever order the -// server felt like". +// filterTools applies AllowTools and MaxTools, drops nameless entries and +// truncates descriptions. +// +// Over the cap WITHOUT allow_tools, the whole contribution is dropped. Taking +// the first N of a sorted list was deterministic but it handed the choice of +// which N to the server: a thirteenth tool named "aaa_" would push a tool that +// had already been discovered, proposed and maybe enabled out of the +// catalogue. Determinism was not the property worth buying. With allow_tools +// set, Kami named the tools, so the cap trims a list he chose. func filterTools(cfg ServerConfig, in []Tool) []Tool { sort.Slice(in, func(i, j int) bool { return in[i].Name < in[j].Name }) out := make([]Tool, 0, len(in)) @@ -259,16 +304,32 @@ func filterTools(cfg ServerConfig, in []Tool) []Tool { if len(cfg.AllowTools) > 0 && !contains(cfg.AllowTools, t.Name) { continue } + t.Description = truncate(t.Description, DefaultMaxDescription) out = append(out, t) } if cfg.MaxTools > 0 && len(out) > cfg.MaxTools { - log.Printf("mcp: %s offers %d tools, taking the first %d (raise max_tools or set allow_tools)", - cfg.Name, len(out), cfg.MaxTools) + if len(cfg.AllowTools) == 0 { + log.Printf("mcp: %s offers %d tools, over the cap of %d — taking NONE of them, set allow_tools to choose or raise max_tools", + cfg.Name, len(out), cfg.MaxTools) + return nil + } + log.Printf("mcp: %s: allow_tools names %d tools, over the cap of %d — taking the first %d", + cfg.Name, len(out), cfg.MaxTools, cfg.MaxTools) out = out[:cfg.MaxTools] } return out } +// truncate bounds a server-written string. The ellipsis is there so a reader +// on /tools can tell the text was cut rather than written that way. +func truncate(s string, max int) string { + r := []rune(s) + if len(r) <= max { + return s + } + return strings.TrimSpace(string(r[:max])) + "…" +} + func contains(hay []string, needle string) bool { for _, h := range hay { if h == needle { @@ -281,18 +342,33 @@ func contains(hay []string, needle string) bool { // Refresh re-dials any server that is down, if enough time has passed since the // last attempt. Call it from the daemon's periodic tick — it is cheap when // everything is up. +// The health check itself is done OUTSIDE m.mu, the way Resources already +// does it. alive() reaches into the transport, and a transport waiting on a +// silent subprocess would otherwise hold m.mu for as long as it waits, which +// blocks Tools, Status and Call for every other server too. func (m *Manager) Refresh(ctx context.Context) { now := time.Now() - var stale []string + type candidate struct { + name string + cl *Client + ready bool + } + var cands []candidate m.mu.Lock() for _, name := range m.order { c := m.conns[name] - down := c.client == nil || !c.client.alive() - if down && now.Sub(c.lastTry) >= DefaultReconnectEvery { - stale = append(stale, name) - } + cands = append(cands, candidate{name: name, cl: c.client, ready: now.Sub(c.lastTry) >= c.backoff()}) } m.mu.Unlock() + var stale []string + for _, c := range cands { + if !c.ready { + continue + } + if c.cl == nil || !c.cl.alive() { + stale = append(stale, c.name) + } + } for _, name := range stale { if err := m.dial(ctx, name); err != nil { log.Printf("mcp: %s: reconnect: %v", name, err) @@ -312,6 +388,21 @@ func (m *Manager) Tools() []Tool { return out } +// Connected — the names of servers that are dialed right now. A caller that +// wants to act on a tool's ABSENCE needs this: a tool missing from Tools() +// because its server is down is not a tool the server withdrew. +func (m *Manager) Connected() []string { + m.mu.Lock() + defer m.mu.Unlock() + var out []string + for _, name := range m.order { + if m.conns[name].client != nil { + out = append(out, name) + } + } + return out +} + // Status is one server's health, for the web surface. type Status struct { Name string @@ -367,13 +458,13 @@ func (m *Manager) Call(ctx context.Context, server, tool string, args map[string } m.mu.Unlock() if cl == nil { - return "", fmt.Errorf("mcp: %s is not connected", server) + return "", fmt.Errorf("%w: %s", ErrNotConnected, server) } // The discovered-and-filtered set is the second allowlist: even an enabled // store row cannot reach a tool the server stopped offering, or one // allow_tools excludes. if !known { - return "", fmt.Errorf("mcp: %s offers no tool %q", server, tool) + return "", fmt.Errorf("%w: %s/%s", ErrToolGone, server, tool) } cctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() @@ -442,8 +533,8 @@ var ErrNeedsArgs = errors.New("mcp: tool needs named arguments") // // - 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; +// - a READ-ONLY tool NAMED IN allow_tools, 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. @@ -456,32 +547,54 @@ var ErrNeedsArgs = errors.New("mcp: tool needs named arguments") // 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. +// +// The allow_tools condition is the second half, and it is there because +// readOnlyHint is the SERVER's claim about itself. It already buys one +// exemption (destructive=false, so no confirm turn); letting it buy argument +// binding as well means one lie converts a spoken utterance into an +// unconfirmed, argument-carrying write. A server advertising delete_project +// with readOnlyHint true and the description "show a project and its tasks" +// would be enough. So the binding half rests on something local instead: a +// name Kami typed into mavend.json. The tool name is not a defence — the +// router picks tools by name similarity and the description a human reads is +// server-written too. 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 + found, readOnly, bindable := false, false, false + configured, connected := c != nil, false if c != nil { + connected = c.client != nil for _, t := range c.tools { if t.Name == tool { schema, readOnly, found = t.InputSchema, t.ReadOnly, true + bindable = contains(c.cfg.AllowTools, tool) break } } } m.mu.Unlock() if !found { - return "", fmt.Errorf("mcp: %s offers no tool %q", server, tool) + if !configured { + return "", fmt.Errorf("%w: %s", ErrNoServer, server) + } + if !connected { + return "", fmt.Errorf("%w: %s", ErrNotConnected, server) + } + return "", fmt.Errorf("%w: %s/%s", ErrToolGone, server, tool) } - named, err := bindPositional(schema, args, readOnly) + named, err := bindPositional(schema, args, readOnly && bindable) 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) { +// bindPositional implements the rule documented on CallPositional. bind is the +// caller's verdict on whether a guessed argument is allowed at all: read-only +// AND named in allow_tools. +func bindPositional(schema json.RawMessage, args []string, bind bool) (map[string]any, error) { var s struct { Required []string `json:"required"` Properties map[string]struct { @@ -498,14 +611,20 @@ func bindPositional(schema json.RawMessage, args []string, readOnly bool) (map[s 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) + prop, described := s.Properties[name] + if !described { + // required names it, properties does not describe it. The zero + // value would make it a string, which is a guess about a guess. + return nil, fmt.Errorf("%w: %q, which the schema never describes", ErrNeedsArgs, name) + } + if !bind { + return nil, fmt.Errorf("%w: %q, and a guessed argument goes only to a read-only tool named in allow_tools", ErrNeedsArgs, name) } tail := strings.TrimSpace(strings.Join(args, " ")) if tail == "" { return nil, fmt.Errorf("%w: %q", ErrNeedsArgs, name) } - switch s.Properties[name].Type { + switch prop.Type { case "string", "": return map[string]any{name: tail}, nil case "integer", "number": @@ -515,7 +634,7 @@ func bindPositional(schema json.RawMessage, args []string, readOnly bool) (map[s } return map[string]any{name: n}, nil default: - return nil, fmt.Errorf("%w: %q is a %s", ErrNeedsArgs, name, s.Properties[name].Type) + return nil, fmt.Errorf("%w: %q is a %s", ErrNeedsArgs, name, prop.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 0888e25..9c8c966 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -3,6 +3,7 @@ package mcp import ( "context" "encoding/json" + "errors" "fmt" "strings" "sync" @@ -102,7 +103,7 @@ func echoServer() func(string, json.RawMessage) (any, *rpcError) { func dialFake(t *testing.T, p *fakePoster) *Client { t.Helper() - c := newClient("fake", newHTTPTransport(p, "http://example.test/mcp")) + c := newClient("fake", newHTTPTransport(p, "http://example.test/mcp", nil)) if err := c.Initialize(context.Background()); err != nil { t.Fatalf("initialize: %v", err) } @@ -190,7 +191,7 @@ func TestResources(t *testing.T) { } func TestCallBeforeInitializeRefused(t *testing.T) { - c := newClient("fake", newHTTPTransport(&fakePoster{handler: echoServer()}, "http://example.test/mcp")) + c := newClient("fake", newHTTPTransport(&fakePoster{handler: echoServer()}, "http://example.test/mcp", nil)) if _, err := c.CallTool(context.Background(), "read_thing", nil); err != ErrNotInitialized { t.Fatalf("err = %v, want ErrNotInitialized", err) } @@ -217,7 +218,7 @@ func TestHandshakeWithoutProtocolVersionRefused(t *testing.T) { p := &fakePoster{handler: func(m string, _ json.RawMessage) (any, *rpcError) { return map[string]any{"serverInfo": map[string]any{"name": "not-mcp"}}, nil }} - c := newClient("x", newHTTPTransport(p, "http://example.test/mcp")) + c := newClient("x", newHTTPTransport(p, "http://example.test/mcp", nil)) if err := c.Initialize(context.Background()); err == nil { t.Fatal("a reply with no protocolVersion is not an MCP server") } @@ -245,8 +246,8 @@ func TestDecodeFrame(t *testing.T) { name, in, want string wantErr bool }{ - {name: "plain json", in: `{"id":1,"result":{}}`, want: `{"id":1,"result":{}}`}, - {name: "sse single", in: "event: message\ndata: {\"id\":1,\"result\":1}\n\n", want: `{"id":1,"result":1}`}, + {name: "plain json", in: `{"id":2,"result":{}}`, want: `{"id":2,"result":{}}`}, + {name: "sse single", in: "event: message\ndata: {\"id\":2,\"result\":1}\n\n", want: `{"id":2,"result":1}`}, { name: "sse picks the response not the notification", in: "data: {\"method\":\"notifications/progress\"}\n\ndata: {\"id\":2,\"result\":2}\n\n", @@ -254,10 +255,24 @@ func TestDecodeFrame(t *testing.T) { }, {name: "empty", in: " ", wantErr: true}, {name: "sse with no response", in: "data: {\"method\":\"x\"}\n\n", wantErr: true}, + { + // A JSON-RPC REQUEST from the server has an id too. Taking it as + // the response gave a frame with neither result nor error, which + // the client reported as an empty success: the act logged as done + // and the tool never run. + name: "sse server request is not a response", + in: "data: {\"id\":2,\"method\":\"sampling/createMessage\",\"params\":{}}\n\n", + wantErr: true, + }, + { + name: "sse response for another id", + in: "data: {\"id\":9,\"result\":1}\n\n", + wantErr: true, + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got, err := decodeFrame([]byte(tc.in)) + got, err := decodeFrame([]byte(tc.in), 2) if tc.wantErr { if err == nil { t.Fatalf("want error, got %q", got) @@ -368,10 +383,19 @@ func TestManagerAllowToolsAndMaxTools(t *testing.T) { if _, err := m.Call(context.Background(), "fake", "break_thing", nil); err == nil { t.Error("a tool excluded by allow_tools must be unreachable") } + // Over the cap with no allow_tools: NOTHING is taken. Trimming a sorted + // list handed the server the choice of which tools survive — a new tool + // named "aaa_" would push an already-approved one out of the catalogue. m2 := mk(ServerConfig{MaxTools: 1}) defer m2.Close() - if got := m2.Tools(); len(got) != 1 || got[0].Name != "break_thing" { - t.Fatalf("max_tools should keep the first name-sorted tool: %+v", got) + if got := m2.Tools(); len(got) != 0 { + t.Fatalf("over the cap without allow_tools must contribute nothing, got %+v", got) + } + // With allow_tools, Kami chose the list, so the cap trims his list. + m3 := mk(ServerConfig{MaxTools: 1, AllowTools: []string{"break_thing", "read_thing"}}) + defer m3.Close() + if got := m3.Tools(); len(got) != 1 || got[0].Name != "break_thing" { + t.Fatalf("max_tools over allow_tools: %+v", got) } } @@ -563,3 +587,57 @@ func TestCallPositionalThroughManager(t *testing.T) { t.Error("an unknown tool must be refused") } } + +// A guessed argument may only be bound for a tool Kami named in allow_tools. +// readOnlyHint alone was the old rule, and readOnlyHint is written by the same +// server that named the tool: a server advertising delete_project as read-only +// got an unconfirmed argument-carrying call. +func TestBindPositionalNeedsAllowTools(t *testing.T) { + schema := json.RawMessage(`{"required":["query"],"properties":{"query":{"type":"string"}}}`) + if _, err := bindPositional(schema, []string{"tea"}, false); !errors.Is(err, ErrNeedsArgs) { + t.Fatalf("err = %v, want ErrNeedsArgs when the tool is not in allow_tools", err) + } + got, err := bindPositional(schema, []string{"tea"}, true) + if err != nil || got["query"] != "tea" { + t.Fatalf("bind = %v, %v", got, err) + } +} + +// required names a property the schema never describes. Falling through to the +// zero value made it a string, which is a guess about a guess. +func TestBindPositionalRefusesUndescribedProperty(t *testing.T) { + schema := json.RawMessage(`{"required":["query"],"properties":{}}`) + _, err := bindPositional(schema, []string{"tea"}, true) + if !errors.Is(err, ErrNeedsArgs) { + t.Fatalf("err = %v, want ErrNeedsArgs", err) + } + if !strings.Contains(err.Error(), "never describes") { + t.Fatalf("err = %v, want it to name the schema gap", err) + } +} + +// A server that cannot be dialled must be retried more and more slowly. At a +// flat one minute a permanently misconfigured stdio server was re-exec'd 1440 +// times a day forever. +func TestReconnectBackoffGrows(t *testing.T) { + c := &conn{} + prev := time.Duration(0) + for i := 1; i <= 12; i++ { + c.fails = i + d := c.backoff() + if d < prev { + t.Fatalf("backoff shrank at %d failures: %v after %v", i, d, prev) + } + if d > MaxReconnectEvery { + t.Fatalf("backoff %v exceeds the cap %v", d, MaxReconnectEvery) + } + prev = d + } + if prev != MaxReconnectEvery { + t.Fatalf("backoff never reached the cap: %v", prev) + } + c.fails = 1 + if c.backoff() != DefaultReconnectEvery { + t.Fatalf("first retry = %v, want %v", c.backoff(), DefaultReconnectEvery) + } +} diff --git a/internal/mcp/webfetchdoor.go b/internal/mcp/webfetchdoor.go index 3a892a2..2623592 100644 --- a/internal/mcp/webfetchdoor.go +++ b/internal/mcp/webfetchdoor.go @@ -16,10 +16,20 @@ import ( // must not become a hole for some public endpoint that happens to redirect at // the LAN. Rate limiting is per fetcher too, which is the right shape here: // separate servers are separate hosts. +// +// A server WITH allow_private also gets redirects switched off. Across servers +// the per-fetcher split holds the line; within the one server that has the +// flag it did not, because allow_private disables the dialer guard on every +// hop: http://localhost:9100/mcp answering 302 to +// http://169.254.169.254/latest/meta-data/ was followed, up to MaxRedirects. A +// local MCP endpoint has no business redirecting, so refusing costs nothing. func WebfetchDoor(limits webfetch.Config) PosterFactory { return func(cfg ServerConfig) (Poster, error) { c := limits c.AllowPrivate = cfg.AllowPrivate + if cfg.AllowPrivate { + c.MaxRedirects = -1 // negative ⇒ no redirects followed + } if c.Timeout <= 0 && cfg.Timeout > 0 { c.Timeout = cfg.Timeout }