fix: fuzzy tool name resolution for LLM tool call mismatches
LLMs (e.g. MiniMax) sometimes call "readfile" instead of "read_file". Add NormalizeToolName to strip underscores/hyphens and lowercase, with fuzzy fallback in ToolRegistry.Get(). Also normalize the interview tool allow-list so blocked tools aren't bypassed by name variants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
b8a8f953e3
commit
4c47a99212
4 changed files with 156 additions and 6 deletions
|
|
@ -1584,16 +1584,19 @@ func isPlanPreExecution(status string) bool {
|
||||||
// isToolAllowedDuringInterview checks whether a tool call is permitted while the
|
// isToolAllowedDuringInterview checks whether a tool call is permitted while the
|
||||||
// plan is in a pre-execution state. Read-type tools are always allowed. Write-type
|
// plan is in a pre-execution state. Read-type tools are always allowed. Write-type
|
||||||
// tools (edit_file, append_file, write_file) are only allowed when targeting MEMORY.md.
|
// tools (edit_file, append_file, write_file) are only allowed when targeting MEMORY.md.
|
||||||
|
// Uses normalized names so "readfile" matches "read_file", etc.
|
||||||
func isToolAllowedDuringInterview(toolName string, args map[string]interface{}) bool {
|
func isToolAllowedDuringInterview(toolName string, args map[string]interface{}) bool {
|
||||||
|
norm := tools.NormalizeToolName(toolName)
|
||||||
|
|
||||||
// Read-type tools: always allowed
|
// Read-type tools: always allowed
|
||||||
switch toolName {
|
switch norm {
|
||||||
case "read_file", "list_dir", "web_search", "web_fetch":
|
case "readfile", "listdir", "websearch", "webfetch":
|
||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
|
|
||||||
// Write-type tools: allowed only when targeting MEMORY.md
|
// Write-type tools: allowed only when targeting MEMORY.md
|
||||||
switch toolName {
|
switch norm {
|
||||||
case "edit_file", "append_file", "write_file":
|
case "editfile", "appendfile", "writefile":
|
||||||
path, _ := args["path"].(string)
|
path, _ := args["path"].(string)
|
||||||
return strings.HasSuffix(path, "MEMORY.md")
|
return strings.HasSuffix(path, "MEMORY.md")
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1319,3 +1319,39 @@ Test
|
||||||
t.Error("expected plan to be cleared after completion")
|
t.Error("expected plan to be cleared after completion")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestIsToolAllowedDuringInterview_FuzzyNames(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
args map[string]interface{}
|
||||||
|
want bool
|
||||||
|
}{
|
||||||
|
// Exact names — read tools allowed
|
||||||
|
{"read_file", nil, true},
|
||||||
|
{"list_dir", nil, true},
|
||||||
|
{"web_search", nil, true},
|
||||||
|
{"web_fetch", nil, true},
|
||||||
|
// Fuzzy variants — should also be allowed
|
||||||
|
{"readfile", nil, true},
|
||||||
|
{"ReadFile", nil, true},
|
||||||
|
{"listdir", nil, true},
|
||||||
|
{"websearch", nil, true},
|
||||||
|
{"webfetch", nil, true},
|
||||||
|
// Write to MEMORY.md — allowed
|
||||||
|
{"edit_file", map[string]interface{}{"path": "/ws/memory/MEMORY.md"}, true},
|
||||||
|
{"editfile", map[string]interface{}{"path": "/ws/memory/MEMORY.md"}, true},
|
||||||
|
{"EditFile", map[string]interface{}{"path": "/ws/memory/MEMORY.md"}, true},
|
||||||
|
// Write to non-MEMORY.md — blocked
|
||||||
|
{"edit_file", map[string]interface{}{"path": "/ws/main.go"}, false},
|
||||||
|
{"editfile", map[string]interface{}{"path": "/ws/main.go"}, false},
|
||||||
|
// exec — always blocked
|
||||||
|
{"exec", nil, false},
|
||||||
|
{"Exec", nil, false},
|
||||||
|
}
|
||||||
|
for _, tt := range tests {
|
||||||
|
got := isToolAllowedDuringInterview(tt.name, tt.args)
|
||||||
|
if got != tt.want {
|
||||||
|
t.Errorf("isToolAllowedDuringInterview(%q, %v) = %v, want %v", tt.name, tt.args, got, tt.want)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -3,6 +3,7 @@ package tools
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"strings"
|
||||||
"sync"
|
"sync"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
|
|
@ -10,6 +11,16 @@ import (
|
||||||
"github.com/sipeed/picoclaw/pkg/providers"
|
"github.com/sipeed/picoclaw/pkg/providers"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
// NormalizeToolName strips underscores and hyphens and lowercases for
|
||||||
|
// fuzzy tool name matching. LLMs sometimes call "readfile" instead of
|
||||||
|
// "read_file", etc.
|
||||||
|
func NormalizeToolName(s string) string {
|
||||||
|
s = strings.ToLower(s)
|
||||||
|
s = strings.ReplaceAll(s, "_", "")
|
||||||
|
s = strings.ReplaceAll(s, "-", "")
|
||||||
|
return s
|
||||||
|
}
|
||||||
|
|
||||||
type ToolRegistry struct {
|
type ToolRegistry struct {
|
||||||
tools map[string]Tool
|
tools map[string]Tool
|
||||||
mu sync.RWMutex
|
mu sync.RWMutex
|
||||||
|
|
@ -30,8 +41,18 @@ func (r *ToolRegistry) Register(tool Tool) {
|
||||||
func (r *ToolRegistry) Get(name string) (Tool, bool) {
|
func (r *ToolRegistry) Get(name string) (Tool, bool) {
|
||||||
r.mu.RLock()
|
r.mu.RLock()
|
||||||
defer r.mu.RUnlock()
|
defer r.mu.RUnlock()
|
||||||
tool, ok := r.tools[name]
|
// Exact match first
|
||||||
return tool, ok
|
if tool, ok := r.tools[name]; ok {
|
||||||
|
return tool, true
|
||||||
|
}
|
||||||
|
// Fuzzy fallback: normalize and compare (handles "readfile" → "read_file" etc.)
|
||||||
|
norm := NormalizeToolName(name)
|
||||||
|
for _, tool := range r.tools {
|
||||||
|
if NormalizeToolName(tool.Name()) == norm {
|
||||||
|
return tool, true
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return nil, false
|
||||||
}
|
}
|
||||||
|
|
||||||
func (r *ToolRegistry) Execute(ctx context.Context, name string, args map[string]interface{}) *ToolResult {
|
func (r *ToolRegistry) Execute(ctx context.Context, name string, args map[string]interface{}) *ToolResult {
|
||||||
|
|
|
||||||
90
pkg/tools/registry_test.go
Normal file
90
pkg/tools/registry_test.go
Normal file
|
|
@ -0,0 +1,90 @@
|
||||||
|
package tools
|
||||||
|
|
||||||
|
import (
|
||||||
|
"context"
|
||||||
|
"testing"
|
||||||
|
)
|
||||||
|
|
||||||
|
// stubTool is a minimal Tool implementation for testing.
|
||||||
|
type stubTool struct {
|
||||||
|
name string
|
||||||
|
}
|
||||||
|
|
||||||
|
func (s *stubTool) Name() string { return s.name }
|
||||||
|
func (s *stubTool) Description() string { return "stub" }
|
||||||
|
func (s *stubTool) Parameters() map[string]interface{} {
|
||||||
|
return map[string]interface{}{"type": "object", "properties": map[string]interface{}{}}
|
||||||
|
}
|
||||||
|
func (s *stubTool) Execute(ctx context.Context, args map[string]interface{}) *ToolResult {
|
||||||
|
return &ToolResult{ForLLM: "ok"}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestNormalizeToolName(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
input, want string
|
||||||
|
}{
|
||||||
|
{"read_file", "readfile"},
|
||||||
|
{"readfile", "readfile"},
|
||||||
|
{"ReadFile", "readfile"},
|
||||||
|
{"read-file", "readfile"},
|
||||||
|
{"edit_file", "editfile"},
|
||||||
|
{"web_search", "websearch"},
|
||||||
|
{"EXEC", "exec"},
|
||||||
|
}
|
||||||
|
for _, tt := range tests {
|
||||||
|
got := NormalizeToolName(tt.input)
|
||||||
|
if got != tt.want {
|
||||||
|
t.Errorf("NormalizeToolName(%q) = %q, want %q", tt.input, got, tt.want)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRegistryGet_ExactMatch(t *testing.T) {
|
||||||
|
r := NewToolRegistry()
|
||||||
|
r.Register(&stubTool{name: "read_file"})
|
||||||
|
|
||||||
|
tool, ok := r.Get("read_file")
|
||||||
|
if !ok || tool.Name() != "read_file" {
|
||||||
|
t.Errorf("exact match failed")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRegistryGet_FuzzyMatch(t *testing.T) {
|
||||||
|
r := NewToolRegistry()
|
||||||
|
r.Register(&stubTool{name: "read_file"})
|
||||||
|
r.Register(&stubTool{name: "edit_file"})
|
||||||
|
r.Register(&stubTool{name: "web_search"})
|
||||||
|
|
||||||
|
tests := []struct {
|
||||||
|
query string
|
||||||
|
wantName string
|
||||||
|
}{
|
||||||
|
{"readfile", "read_file"},
|
||||||
|
{"ReadFile", "read_file"},
|
||||||
|
{"read-file", "read_file"},
|
||||||
|
{"editfile", "edit_file"},
|
||||||
|
{"EditFile", "edit_file"},
|
||||||
|
{"websearch", "web_search"},
|
||||||
|
{"WebSearch", "web_search"},
|
||||||
|
}
|
||||||
|
for _, tt := range tests {
|
||||||
|
tool, ok := r.Get(tt.query)
|
||||||
|
if !ok {
|
||||||
|
t.Errorf("Get(%q) not found, want %q", tt.query, tt.wantName)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if tool.Name() != tt.wantName {
|
||||||
|
t.Errorf("Get(%q).Name() = %q, want %q", tt.query, tool.Name(), tt.wantName)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRegistryGet_NotFound(t *testing.T) {
|
||||||
|
r := NewToolRegistry()
|
||||||
|
r.Register(&stubTool{name: "read_file"})
|
||||||
|
|
||||||
|
_, ok := r.Get("totally_unknown")
|
||||||
|
if ok {
|
||||||
|
t.Errorf("Get(totally_unknown) should return false")
|
||||||
|
}
|
||||||
|
}
|
||||||
Loading…
Add table
Reference in a new issue