From fa219e06aa49f264e05feb379ce701038e9187b6 Mon Sep 17 00:00:00 2001 From: Levi Neely Date: Wed, 26 Aug 2026 14:00:21 +0200 Subject: [PATCH] quirks: only reject shell calls with tools in command position The old word-boundary check falsely triggered when tool names appeared in arguments (e.g., git commit messages mentioning native tools). New logic: split on shell separators and only flag when a tool is the first token of a sub-command or a path ending with /toolname. --- cmd/toolsrv/internal/quirks/shell.go | 109 ++++++++++++++++------ cmd/toolsrv/internal/quirks/shell_test.go | 56 +++++++++++ 2 files changed, 137 insertions(+), 28 deletions(-) create mode 100644 cmd/toolsrv/internal/quirks/shell_test.go diff --git a/cmd/toolsrv/internal/quirks/shell.go b/cmd/toolsrv/internal/quirks/shell.go index 30c7d93..9b7ca9d 100644 --- a/cmd/toolsrv/internal/quirks/shell.go +++ b/cmd/toolsrv/internal/quirks/shell.go @@ -3,49 +3,102 @@ package quirks import "strings" -// ShellInvokesNativeTool checks if a shell command contains a native tool name. -// Returns the tool name if found, empty string otherwise. +// ShellInvokesNativeTool checks if a shell command attempts to execute a native +// tool. Returns the tool name if found, empty string otherwise. // -// This is a hack for broken models that ignore prompt instructions and shell -// out to tools instead of calling them natively. +// Only matches when the tool name appears in command position: +// - First word of the command +// - After a command separator (; | && ||) +// - As a path ending with the tool name (./tool, /usr/bin/tool) +// +// Does NOT match tool names that merely appear in arguments (e.g., commit messages). func ShellInvokesNativeTool(cmd string, tools []string) string { - // Reject ollie-9p in shell — use client_9p native tool instead. - if strings.Contains(cmd, "ollie-9p") { + // Reject ollie-9p in command position. + if isInCommandPosition(cmd, "ollie-9p") { return "client_9p" } - // Check if cmd contains any tool name as a word boundary match. - // Match: "skill_list", "echo foo | skill_list", "skill_list arg" - // Don't match: "my_skill_list" (substring) for _, name := range tools { if name == "shell" { - continue // don't match shell itself + continue } - if matchesWordBoundary(cmd, name) { + if isInCommandPosition(cmd, name) { return name } } return "" } -// matchesWordBoundary checks if name appears in cmd at word boundaries. -func matchesWordBoundary(cmd, name string) bool { - idx := strings.Index(cmd, name) - if idx < 0 { - return false +// isInCommandPosition returns true if name appears as an executable in cmd. +// It checks whether name is the first token of a (sub-)command or appears as a +// path ending with /name. +func isInCommandPosition(cmd, name string) bool { + // Check each sub-command (split on shell separators). + for _, sub := range splitCommands(cmd) { + sub = strings.TrimSpace(sub) + if sub == "" { + continue + } + // Get the first token (the command being executed). + first := firstToken(sub) + // Exact match: "tool_name args..." + if first == name { + return true + } + // Path match: "./tool_name", "../tool_name", "/usr/bin/tool_name" + if strings.HasSuffix(first, "/"+name) { + return true + } } - // Check left boundary: start of string or non-word char - if idx > 0 && isWordChar(cmd[idx-1]) { - return false - } - // Check right boundary: end of string or non-word char - end := idx + len(name) - if end < len(cmd) && isWordChar(cmd[end]) { - return false - } - return true + return false } -func isWordChar(c byte) bool { - return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' +// splitCommands splits a shell command line on command separators: ;, |, &&, || +// This is a rough heuristic — it doesn't handle quoting, but it's good enough +// to identify command positions in typical AI-generated shell commands. +func splitCommands(cmd string) []string { + var parts []string + var current strings.Builder + i := 0 + for i < len(cmd) { + switch { + case cmd[i] == ';' || cmd[i] == '|': + parts = append(parts, current.String()) + current.Reset() + if cmd[i] == '|' && i+1 < len(cmd) && cmd[i+1] == '|' { + i++ // skip || + } + i++ + case cmd[i] == '&' && i+1 < len(cmd) && cmd[i+1] == '&': + parts = append(parts, current.String()) + current.Reset() + i += 2 + case cmd[i] == '$' && i+1 < len(cmd) && cmd[i+1] == '(': + // Command substitution — the content after $( is a new command + parts = append(parts, current.String()) + current.Reset() + i += 2 + case cmd[i] == '`': + // Backtick substitution + parts = append(parts, current.String()) + current.Reset() + i++ + default: + current.WriteByte(cmd[i]) + i++ + } + } + if current.Len() > 0 { + parts = append(parts, current.String()) + } + return parts +} + +// firstToken returns the first whitespace-delimited token from s. +func firstToken(s string) string { + s = strings.TrimSpace(s) + if i := strings.IndexAny(s, " \t"); i >= 0 { + return s[:i] + } + return s } diff --git a/cmd/toolsrv/internal/quirks/shell_test.go b/cmd/toolsrv/internal/quirks/shell_test.go new file mode 100644 index 0000000..28a37d6 --- /dev/null +++ b/cmd/toolsrv/internal/quirks/shell_test.go @@ -0,0 +1,56 @@ +package quirks + +import "testing" + +func TestShellInvokesNativeTool(t *testing.T) { + tools := []string{"shell", "client_9p", "file_read", "file_edit", "skill_list", "memory_wake"} + + tests := []struct { + cmd string + want string + }{ + // Should match: tool in command position + {"client_9p read foo", "client_9p"}, + {"file_read path=/tmp/x", "file_read"}, + {"skill_list", "skill_list"}, + {"echo hello | skill_list", "skill_list"}, + {"echo hello && file_read foo", "file_read"}, + {"echo hello || file_edit bar", "file_edit"}, + {"echo hello; memory_wake", "memory_wake"}, + {"./client_9p read foo", "client_9p"}, + {"/usr/local/bin/client_9p read", "client_9p"}, + {"../bin/file_read foo", "file_read"}, + {"$(file_read foo)", "file_read"}, + {"`skill_list`", "skill_list"}, + + // Should match: ollie-9p -> client_9p + {"ollie-9p read session/foo", "client_9p"}, + {"echo x | ollie-9p write foo", "client_9p"}, + + // Should NOT match: tool name in arguments (not command position) + {"git commit -m 'use client_9p interface'", ""}, + {"echo 'client_9p is great'", ""}, + {"git commit -m 'add file_read support'", ""}, + {"grep skill_list README.md", ""}, + {"cat file_read.go", ""}, + {"echo ollie-9p is the CLI", ""}, + + // Should NOT match: substring + {"my_client_9p_wrapper foo", ""}, + + // Should not match shell itself + {"shell command", ""}, + + // Edge cases + {"", ""}, + {"ls -la", ""}, + {"go build ./...", ""}, + } + + for _, tt := range tests { + got := ShellInvokesNativeTool(tt.cmd, tools) + if got != tt.want { + t.Errorf("ShellInvokesNativeTool(%q) = %q, want %q", tt.cmd, got, tt.want) + } + } +}