From b2f239c6518c3ff5b4b13490842ad6c0ccc0737b Mon Sep 17 00:00:00 2001 From: stefanwalcz Date: Tue, 7 Jul 2026 16:05:39 +0200 Subject: [PATCH] pickTool: name the chosen tool in the error and match tool names leniently Local models routinely call a tool by a variant of its name ("Bash", "functions.bash", "tools/bash", "coach-regeln"). Tools.Find now falls back to a normalized match when there is no exact one, and a pickTool miss names the tool the model chose. The lenient match is made safe for embedders that gate tool calls by name (review on #64): - Before any tool-call callback runs, every selected tool's name is rewritten to the real name of the tool it resolves to, in pickTool and for native tool calls, so what gets approved is what gets executed. A model that calls "Bash" no longer runs "bash" past an approval hook configured for "bash". - A lenient match only counts when exactly one tool matches. When two or more tools normalize to the same name ("github/list_issues" and "gitlab/list_issues"), the name is not found and the error names the candidates. Tests: the callback receives the canonical name for lenient matches (three spellings, end to end through ExecuteTools); an ambiguous name resolves to nothing, the error names both candidates, and end to end neither tool runs. Without the canonicalization the callback test sees the model's spelling; without the uniqueness rule the ambiguous tests pick and run the first tool. Signed-off-by: Stefan Walcz Assisted-by: Claude:claude-opus-5-5 --- tools.go | 80 ++++++++++++++++++++++++- tools_lenient_test.go | 136 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 213 insertions(+), 3 deletions(-) create mode 100644 tools_lenient_test.go diff --git a/tools.go b/tools.go index b361454..8c5bd47 100644 --- a/tools.go +++ b/tools.go @@ -133,15 +133,82 @@ func (t *ToolDefinition[T]) Execute(args map[string]any) (string, any, error) { type Tools []ToolDefinitionInterface +// Find returns the tool whose function name matches name. It first tries an +// exact match (fast path, unchanged behavior). If none is found, it falls back +// to a lenient match: local models frequently emit namespaced/case/separator +// variants (e.g. "functions.foo", "tools/foo", "Foo", "foo-bar" for "foo_bar"). +// The lenient pass never overrides an exact match, so existing resolutions are +// unaffected. func (t Tools) Find(name string) ToolDefinitionInterface { for _, tool := range t { if tool.Tool().Function.Name == name { return tool } } + if matches := t.lenientMatches(name); len(matches) == 1 { + return matches[0] + } return nil } +// lenientMatches returns the tools whose normalized name equals the +// normalized form of name. Find only accepts a lenient match when exactly one +// tool matches: "github/list_issues" and "gitlab/list_issues" both normalize to +// "list_issues", and picking whichever comes first would run the wrong tool. +func (t Tools) lenientMatches(name string) []ToolDefinitionInterface { + norm := normalizeToolName(name) + if norm == "" { + return nil + } + var matches []ToolDefinitionInterface + for _, tool := range t { + if normalizeToolName(tool.Tool().Function.Name) == norm { + matches = append(matches, tool) + } + } + return matches +} + +// notFoundError explains why name did not resolve to a tool, naming the +// candidates when a lenient match was ambiguous. +func (t Tools) notFoundError(name string) error { + if matches := t.lenientMatches(name); len(matches) > 1 { + names := make([]string, 0, len(matches)) + for _, m := range matches { + names = append(names, m.Tool().Function.Name) + } + return fmt.Errorf("chosen tool %q not found: ambiguous, matches %s", name, strings.Join(names, ", ")) + } + return fmt.Errorf("chosen tool %q not found", name) +} + +// canonicalizeToolChoice rewrites tc.Name to the real name of the tool it +// resolves to. Tool-call callbacks approve by name and execution resolves the +// name again, so both must see the same, canonical name: otherwise a model that +// calls "Bash" would get "bash" executed while an approval hook configured for +// "bash" never sees it. +func (t Tools) canonicalizeToolChoice(tc *ToolChoice) { + if tc == nil { + return + } + if tool := t.Find(tc.Name); tool != nil { + tc.Name = tool.Tool().Function.Name + } +} + +// normalizeToolName reduces a tool name to a comparison key: it drops a leading +// namespace segment ("functions.", "tools/", "namespace::"), lower-cases, trims +// spaces, and treats '-' and '_' as equivalent. Used only as a lenient fallback +// in Find; it never affects exact-match resolution. +func normalizeToolName(s string) string { + if i := strings.LastIndexAny(s, "./:"); i >= 0 { + s = s[i+1:] // segment after the last separator (may be empty → empty key) + } + s = strings.ToLower(strings.TrimSpace(s)) + s = strings.ReplaceAll(s, "-", "_") + return s +} + func (t Tools) ToOpenAI() []openai.Tool { openaiTools := []openai.Tool{} for _, tool := range t { @@ -835,7 +902,7 @@ func pickTool(ctx context.Context, llm LLM, fragment Fragment, tools Tools, opts } toolChoices = append(toolChoices, &ToolChoice{ - Name: toolName, + Name: chosenTool.Tool().Function.Name, Arguments: make(map[string]any), Reasoning: intentionReasoning, }) @@ -861,11 +928,11 @@ func pickTool(ctx context.Context, llm LLM, fragment Fragment, tools Tools, opts chosenTool := tools.Find(intentionResponse.Tool) if chosenTool == nil { xlog.Debug("[pickTool] Chosen tool not found", "tool", intentionResponse.Tool) - return nil, fmt.Errorf("chosen tool not found") + return nil, tools.notFoundError(intentionResponse.Tool) } toolChoices = append(toolChoices, &ToolChoice{ - Name: intentionResponse.Tool, + Name: chosenTool.Tool().Function.Name, Arguments: make(map[string]any), Reasoning: intentionReasoning, }) @@ -1614,6 +1681,13 @@ TOOL_LOOP: xlog.Debug("Picked tools with args", "count", len(selectedToolResults)) + // Canonicalize every selected tool name before the sink-state check and + // the tool-call callbacks, so what gets approved is what gets executed + // (native tool calls carry the model's own spelling). + for _, toolResult := range selectedToolResults { + tools.canonicalizeToolChoice(toolResult) + } + // Check for sink state and separate tools var toolsToExecute []*ToolChoice sinkStateName := "" diff --git a/tools_lenient_test.go b/tools_lenient_test.go new file mode 100644 index 0000000..436b4b4 --- /dev/null +++ b/tools_lenient_test.go @@ -0,0 +1,136 @@ +package cogito + +import ( + "strings" + "sync" + "testing" +) + +func TestNormalizeToolName(t *testing.T) { + cases := []struct{ in, want string }{ + {"foo", "foo"}, + {"Foo", "foo"}, + {"functions.foo", "foo"}, + {"tools/foo", "foo"}, + {"namespace::foo", "foo"}, + {"foo-bar", "foo_bar"}, + {"Foo-Bar", "foo_bar"}, + {"functions.Coach_Regeln", "coach_regeln"}, + {" foo ", "foo"}, + {"", ""}, + {"functions.", ""}, // nothing after the separator → empty key + } + for _, c := range cases { + if got := normalizeToolName(c.in); got != c.want { + t.Errorf("normalizeToolName(%q) = %q, want %q", c.in, got, c.want) + } + } +} + +func TestFindLenient(t *testing.T) { + tools := Tools{newNamedTool("coach_regeln")} + + // exact + lenient variants local models routinely emit + for _, n := range []string{ + "coach_regeln", // exact + "functions.coach_regeln", // namespaced + "tools/coach_regeln", + "Coach_Regeln", // case + "coach-regeln", // separator + } { + if tools.Find(n) == nil { + t.Errorf("Find(%q) = nil, want match", n) + } + } + + for _, n := range []string{"other_tool", "", "functions."} { + if tools.Find(n) != nil { + t.Errorf("Find(%q) matched, want nil", n) + } + } +} + +// An exact match must always win, even when a lenient candidate appears earlier +// in the slice — the lenient pass is a fallback, never an override. +func TestFindExactWinsOverLenient(t *testing.T) { + tools := Tools{newNamedTool("functions.foo"), newNamedTool("foo")} + got := tools.Find("foo") + if got == nil || got.Tool().Function.Name != "foo" { + name := "" + if got != nil { + name = got.Tool().Function.Name + } + t.Errorf(`Find("foo") = %q, want exact "foo"`, name) + } +} + +// A lenient match must hand the tool-call callback the CANONICAL tool name — +// the name that is executed — not the model's spelling. Otherwise an approval +// hook configured for "echo" would never see a call the model spelled "Echo". +func TestLenientToolCallCallbackSeesCanonicalName(t *testing.T) { + for _, spelled := range []string{"Echo", "functions.echo", "tools/echo"} { + t.Run(spelled, func(t *testing.T) { + var mu sync.Mutex + var seen []string + cb := func(tc *ToolChoice, _ *SessionState) ToolCallDecision { + mu.Lock() + seen = append(seen, tc.Name) + mu.Unlock() + return ToolCallDecision{Approved: true} + } + var echoMu sync.Mutex + echoCount := 0 + llm := newScriptedLLM(spelled, `{"text": "hi"}`, "done") + f := NewEmptyFragment().AddMessage(UserMessageRole, "say hi") + if _, err := ExecuteTools(llm, f, WithTools(newEchoTool(&echoMu, &echoCount)), WithToolCallBack(cb), WithIterations(1)); err != nil && err != ErrNoToolSelected { + t.Fatalf("ExecuteTools: %v", err) + } + if len(seen) != 1 || seen[0] != "echo" { + t.Fatalf("callback saw %v, want [echo]", seen) + } + if echoCount != 1 { + t.Fatalf("echo ran %d times, want 1", echoCount) + } + }) + } +} + +// When two tools normalize to the same name, a lenient lookup must not pick +// either one: it is not found, and the error names the candidates. +func TestFindAmbiguousLenientIsNotFound(t *testing.T) { + tools := Tools{newNamedTool("github/list_issues"), newNamedTool("gitlab/list_issues")} + + if got := tools.Find("list_issues"); got != nil { + t.Fatalf(`Find("list_issues") = %q, want nil (ambiguous)`, got.Tool().Function.Name) + } + if got := tools.Find("functions.list_issues"); got != nil { + t.Fatalf(`Find("functions.list_issues") = %q, want nil (ambiguous)`, got.Tool().Function.Name) + } + // exact names still resolve + for _, n := range []string{"github/list_issues", "gitlab/list_issues"} { + if got := tools.Find(n); got == nil || got.Tool().Function.Name != n { + t.Fatalf("Find(%q) did not return the exact tool", n) + } + } + err := tools.notFoundError("list_issues") + if err == nil || !strings.Contains(err.Error(), "github/list_issues") || !strings.Contains(err.Error(), "gitlab/list_issues") { + t.Fatalf("notFoundError = %v, want both candidates named", err) + } + if e := (Tools{newNamedTool("foo")}).notFoundError("bar"); e == nil || strings.Contains(e.Error(), "ambiguous") { + t.Fatalf("plain not-found error = %v, want no ambiguity note", e) + } +} + +// End to end: a model call to an ambiguous name must execute neither tool. +func TestAmbiguousLenientToolCallExecutesNothing(t *testing.T) { + var mu sync.Mutex + a, b := 0, 0 + toolA := NewToolDefinition[EchoArgs](echoRunner{mu: &mu, count: &a}, EchoArgs{}, "github/list_issues", "a") + toolB := NewToolDefinition[EchoArgs](echoRunner{mu: &mu, count: &b}, EchoArgs{}, "gitlab/list_issues", "b") + llm := newScriptedLLM("list_issues", `{"text": "x"}`, "done") + f := NewEmptyFragment().AddMessage(UserMessageRole, "list issues") + _, _ = ExecuteTools(llm, f, WithTools(toolA, toolB), WithIterations(1)) + if a != 0 || b != 0 { + t.Fatalf("ambiguous name executed a tool (github=%d, gitlab=%d), want neither", a, b) + } +}