From 52f56947bbf1941b77d036b6a0b2e90790e74980 Mon Sep 17 00:00:00 2001 From: kami Date: Sat, 1 Aug 2026 14:11:57 +0400 Subject: [PATCH] tool: separate a tool that is off from a backend that is down An enabled MCP or smarthome row with no backend returned ErrNotEnabled, and actionAct reads ErrNotEnabled as "this is unknown, draft a proposal". So a tool Kami had already approved, whose server happened to be restarting, produced a second proposal row and an answer saying the tool needs approval. The right answer is that the server is down. ErrNotConnected carries that, and the act path maps it, ErrNoServer and ErrToolGone to replies that say which of the three happened. Found in review of #71. --- cmd/mavend/actions_act.go | 7 +++++++ internal/tool/tool.go | 11 +++++++++-- internal/tool/tool_test.go | 15 +++++++++++---- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/cmd/mavend/actions_act.go b/cmd/mavend/actions_act.go index be83e9e..2b96855 100644 --- a/cmd/mavend/actions_act.go +++ b/cmd/mavend/actions_act.go @@ -53,6 +53,13 @@ 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, tool.ErrNotConnected), errors.Is(err, mcp.ErrNotConnected), errors.Is(err, mcp.ErrNoServer): + // The row is enabled and the backend is gone. Drafting a proposal + // for it (the ErrNotEnabled path) would be answering the wrong + // question. + return "этот инструмент включён, но сервер, который его выполняет, сейчас не подключён." + case errors.Is(err, mcp.ErrToolGone): + return "сервер больше не предлагает этот инструмент — я сняла его с разрешённых, посмотри на /tools." 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 diff --git a/internal/tool/tool.go b/internal/tool/tool.go index 4c0165d..3b16c59 100644 --- a/internal/tool/tool.go +++ b/internal/tool/tool.go @@ -60,6 +60,13 @@ var ( ErrNotEnabled = errors.New("tool not on the enabled allowlist") // ErrNeedsConfirm — the fn is enabled but destructive; needs a confirm turn. ErrNeedsConfirm = errors.New("destructive tool needs confirmation") + // ErrNotConnected — the row is enabled and well formed, but the thing it + // dispatches to is not wired: the mcp block was dropped from the config + // while enabled MCP rows remained, or the same for the house. Held apart + // from ErrNotEnabled because the act path turns that one into a fresh + // proposal, and drafting a new proposal for a tool that already exists and + // is enabled is a lie about what is wrong. + ErrNotConnected = errors.New("tool is enabled but its backend is not connected") ) // MCPCaller is the seam for an act that is an MCP tool call rather than a @@ -134,7 +141,7 @@ func (e *Executor) Exec(ctx context.Context, name string, args []string, confirm // confirmed. Only the dispatch differs. if server, remote, ok := mcp.ParseCmd(t.Cmd); ok { if e.mcp == nil { - return "", ErrNotEnabled + return "", fmt.Errorf("%w: %s is an MCP tool and no mcp block is configured", ErrNotConnected, name) } ctx, cancel := context.WithTimeout(ctx, e.timeout) defer cancel() @@ -148,7 +155,7 @@ func (e *Executor) Exec(ctx context.Context, name string, args []string, confirm // row but can never compose a target of its own. if entityID, service, ok := smarthome.ParseCmd(t.Cmd); ok { if e.home == nil { - return "", ErrNotEnabled + return "", fmt.Errorf("%w: %s is a house tool and no smarthome block is configured", ErrNotConnected, name) } ctx, cancel := context.WithTimeout(ctx, e.timeout) defer cancel() diff --git a/internal/tool/tool_test.go b/internal/tool/tool_test.go index cfc4c35..ed65bd6 100644 --- a/internal/tool/tool_test.go +++ b/internal/tool/tool_test.go @@ -178,8 +178,15 @@ func TestExecMCPRowWithoutCallerRefuses(t *testing.T) { 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) + err := func() error { _, e2 := e.Exec(context.Background(), "vikunja_list_tasks", nil, false); return e2 }() + // ErrNotConnected, NOT ErrNotEnabled: the act path turns ErrNotEnabled into + // a fresh proposal, and drafting a proposal for a row that already exists + // and is enabled answers the wrong question. + if !errors.Is(err, ErrNotConnected) { + t.Fatalf("err = %v, want ErrNotConnected", err) + } + if errors.Is(err, ErrNotEnabled) { + t.Fatal("an enabled row with a missing backend must not read as not-enabled") } if ran { t.Fatal(`"mcp" must never be run as a binary`) @@ -222,8 +229,8 @@ func TestExecSmartHomeRow(t *testing.T) { } // No house configured ⇒ the row refuses rather than being exec'd. - if _, err := newExec(nil).Exec(context.Background(), "home_light_x_off", nil, true); !errors.Is(err, ErrNotEnabled) { - t.Fatalf("unconfigured house: err = %v, want ErrNotEnabled", err) + if _, err := newExec(nil).Exec(context.Background(), "home_light_x_off", nil, true); !errors.Is(err, ErrNotConnected) { + t.Fatalf("unconfigured house: err = %v, want ErrNotConnected", err) } if ran { t.Fatal(`"smarthome" was run as a binary`)