From 9b5ce8c64f91cb6af2b34bb9c95eac7ca931c9b2 Mon Sep 17 00:00:00 2001 From: mpfo0106 Date: Fri, 3 Apr 2026 00:13:02 +0900 Subject: [PATCH] Keep Claude builtin helpers aligned with the shared helper layout The review asked for the builtin tool registry helper to live with the rest of executor support utilities. This moves the registry code into the helps package, exports the minimal surface executor needs, and keeps behavior tests with the executor while leaving registry-focused checks with the helper. Constraint: Requested layout keeps executor helper utilities centralized under internal/runtime/executor/helps Rejected: Keep the files in executor and reply with rationale | conflicts with requested package organization Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep executor behavior tests near applyClaudeToolPrefix and keep pure registry tests in helps Tested: go test ./internal/runtime/executor/helps ./internal/runtime/executor -run 'Claude|Builtin|Tool'; go test ./test/...; go test ./... Not-tested: End-to-end Claude Code direct-connect/session runtime behavior --- .../executor/claude_builtin_tools_test.go | 46 ------------------- internal/runtime/executor/claude_executor.go | 2 +- .../runtime/executor/claude_executor_test.go | 29 ++++++++++++ .../{ => helps}/claude_builtin_tools.go | 4 +- .../helps/claude_builtin_tools_test.go | 32 +++++++++++++ 5 files changed, 64 insertions(+), 49 deletions(-) delete mode 100644 internal/runtime/executor/claude_builtin_tools_test.go rename internal/runtime/executor/{ => helps}/claude_builtin_tools.go (90%) create mode 100644 internal/runtime/executor/helps/claude_builtin_tools_test.go diff --git a/internal/runtime/executor/claude_builtin_tools_test.go b/internal/runtime/executor/claude_builtin_tools_test.go deleted file mode 100644 index 34036fa0..00000000 --- a/internal/runtime/executor/claude_builtin_tools_test.go +++ /dev/null @@ -1,46 +0,0 @@ -package executor - -import ( - "fmt" - "testing" - - "github.com/tidwall/gjson" -) - -func TestClaudeBuiltinToolRegistry_DefaultSeedFallback(t *testing.T) { - registry := augmentClaudeBuiltinToolRegistry(nil, nil) - for _, name := range defaultClaudeBuiltinToolNames { - if !registry[name] { - t.Fatalf("default builtin %q missing from fallback registry", name) - } - } -} - -func TestApplyClaudeToolPrefix_KnownFallbackBuiltinsRemainUnprefixed(t *testing.T) { - for _, builtin := range defaultClaudeBuiltinToolNames { - t.Run(builtin, func(t *testing.T) { - input := []byte(fmt.Sprintf(`{ - "tools":[{"name":"Read"}], - "tool_choice":{"type":"tool","name":%q}, - "messages":[{"role":"assistant","content":[{"type":"tool_use","name":%q,"id":"toolu_1","input":{}},{"type":"tool_reference","tool_name":%q},{"type":"tool_result","tool_use_id":"toolu_1","content":[{"type":"tool_reference","tool_name":%q}]}]}] - }`, builtin, builtin, builtin, builtin)) - out := applyClaudeToolPrefix(input, "proxy_") - - if got := gjson.GetBytes(out, "tool_choice.name").String(); got != builtin { - t.Fatalf("tool_choice.name = %q, want %q", got, builtin) - } - if got := gjson.GetBytes(out, "messages.0.content.0.name").String(); got != builtin { - t.Fatalf("messages.0.content.0.name = %q, want %q", got, builtin) - } - if got := gjson.GetBytes(out, "messages.0.content.1.tool_name").String(); got != builtin { - t.Fatalf("messages.0.content.1.tool_name = %q, want %q", got, builtin) - } - if got := gjson.GetBytes(out, "messages.0.content.2.content.0.tool_name").String(); got != builtin { - t.Fatalf("messages.0.content.2.content.0.tool_name = %q, want %q", got, builtin) - } - if got := gjson.GetBytes(out, "tools.0.name").String(); got != "proxy_Read" { - t.Fatalf("tools.0.name = %q, want %q", got, "proxy_Read") - } - }) - } -} diff --git a/internal/runtime/executor/claude_executor.go b/internal/runtime/executor/claude_executor.go index d1d2e136..120b1f35 100644 --- a/internal/runtime/executor/claude_executor.go +++ b/internal/runtime/executor/claude_executor.go @@ -921,7 +921,7 @@ func applyClaudeToolPrefix(body []byte, prefix string) []byte { // Collect built-in tool names from the authoritative fallback seed list and // augment it with any typed built-ins present in the current request body. - builtinTools := augmentClaudeBuiltinToolRegistry(body, nil) + builtinTools := helps.AugmentClaudeBuiltinToolRegistry(body, nil) if tools := gjson.GetBytes(body, "tools"); tools.Exists() && tools.IsArray() { tools.ForEach(func(index, tool gjson.Result) bool { diff --git a/internal/runtime/executor/claude_executor_test.go b/internal/runtime/executor/claude_executor_test.go index 8e8173dd..e5f907b7 100644 --- a/internal/runtime/executor/claude_executor_test.go +++ b/internal/runtime/executor/claude_executor_test.go @@ -739,6 +739,35 @@ func TestApplyClaudeToolPrefix_ToolChoiceBuiltin(t *testing.T) { } } +func TestApplyClaudeToolPrefix_KnownFallbackBuiltinsRemainUnprefixed(t *testing.T) { + for _, builtin := range []string{"web_search", "code_execution", "text_editor", "computer"} { + t.Run(builtin, func(t *testing.T) { + input := []byte(fmt.Sprintf(`{ + "tools":[{"name":"Read"}], + "tool_choice":{"type":"tool","name":%q}, + "messages":[{"role":"assistant","content":[{"type":"tool_use","name":%q,"id":"toolu_1","input":{}},{"type":"tool_reference","tool_name":%q},{"type":"tool_result","tool_use_id":"toolu_1","content":[{"type":"tool_reference","tool_name":%q}]}]}] + }`, builtin, builtin, builtin, builtin)) + out := applyClaudeToolPrefix(input, "proxy_") + + if got := gjson.GetBytes(out, "tool_choice.name").String(); got != builtin { + t.Fatalf("tool_choice.name = %q, want %q", got, builtin) + } + if got := gjson.GetBytes(out, "messages.0.content.0.name").String(); got != builtin { + t.Fatalf("messages.0.content.0.name = %q, want %q", got, builtin) + } + if got := gjson.GetBytes(out, "messages.0.content.1.tool_name").String(); got != builtin { + t.Fatalf("messages.0.content.1.tool_name = %q, want %q", got, builtin) + } + if got := gjson.GetBytes(out, "messages.0.content.2.content.0.tool_name").String(); got != builtin { + t.Fatalf("messages.0.content.2.content.0.tool_name = %q, want %q", got, builtin) + } + if got := gjson.GetBytes(out, "tools.0.name").String(); got != "proxy_Read" { + t.Fatalf("tools.0.name = %q, want %q", got, "proxy_Read") + } + }) + } +} + func TestStripClaudeToolPrefixFromResponse(t *testing.T) { input := []byte(`{"content":[{"type":"tool_use","name":"proxy_alpha","id":"t1","input":{}},{"type":"tool_use","name":"bravo","id":"t2","input":{}}]}`) out := stripClaudeToolPrefixFromResponse(input, "proxy_") diff --git a/internal/runtime/executor/claude_builtin_tools.go b/internal/runtime/executor/helps/claude_builtin_tools.go similarity index 90% rename from internal/runtime/executor/claude_builtin_tools.go rename to internal/runtime/executor/helps/claude_builtin_tools.go index 8c3592f7..5ee2b08d 100644 --- a/internal/runtime/executor/claude_builtin_tools.go +++ b/internal/runtime/executor/helps/claude_builtin_tools.go @@ -1,4 +1,4 @@ -package executor +package helps import "github.com/tidwall/gjson" @@ -17,7 +17,7 @@ func newClaudeBuiltinToolRegistry() map[string]bool { return registry } -func augmentClaudeBuiltinToolRegistry(body []byte, registry map[string]bool) map[string]bool { +func AugmentClaudeBuiltinToolRegistry(body []byte, registry map[string]bool) map[string]bool { if registry == nil { registry = newClaudeBuiltinToolRegistry() } diff --git a/internal/runtime/executor/helps/claude_builtin_tools_test.go b/internal/runtime/executor/helps/claude_builtin_tools_test.go new file mode 100644 index 00000000..d7badd19 --- /dev/null +++ b/internal/runtime/executor/helps/claude_builtin_tools_test.go @@ -0,0 +1,32 @@ +package helps + +import "testing" + +func TestClaudeBuiltinToolRegistry_DefaultSeedFallback(t *testing.T) { + registry := AugmentClaudeBuiltinToolRegistry(nil, nil) + for _, name := range defaultClaudeBuiltinToolNames { + if !registry[name] { + t.Fatalf("default builtin %q missing from fallback registry", name) + } + } +} + +func TestClaudeBuiltinToolRegistry_AugmentsTypedBuiltinsFromBody(t *testing.T) { + registry := AugmentClaudeBuiltinToolRegistry([]byte(`{ + "tools": [ + {"type": "web_search_20250305", "name": "web_search"}, + {"type": "custom_builtin_20250401", "name": "special_builtin"}, + {"name": "Read"} + ] + }`), nil) + + if !registry["web_search"] { + t.Fatal("expected default typed builtin web_search in registry") + } + if !registry["special_builtin"] { + t.Fatal("expected typed builtin from body to be added to registry") + } + if registry["Read"] { + t.Fatal("expected untyped custom tool to stay out of builtin registry") + } +}