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) + } +}