mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-15 08:17:09 +00:00
fix(agent): check nil td.Function to prevent panic on native tools (#1192)
* fix(agent): check nil td.Function to prevent panic on native tools Fix nil pointer dereference panic when agent processes native tools (such as image_generation) which do not contain function schema wrappers. Key changes: - loop_pipeline_callbacks.go & think_stage.go: add checks for td.Function != nil before accessing td.Function.Name. - loop_tool_filter.go & loop_history_toolnames.go: add safety checks when filtering and extracting tool names. - anthropic_request.go & codex_build.go: verify t.Function != nil when formatting tools for provider API requests. * test: add regression tests for native tools nil function panic Add targeted regression tests to verify that: - ThinkStage builds AllowedTools without panicking when mixed with native tools - Loop's buildFilteredTools filters native tools correctly without panicking across all filters - Anthropic request builder skips native tools safely - Codex request builder handles native tools with nil Function fields gracefully
This commit is contained in:
1 parent
db98e477d4
commit
389640ae51
10 files changed
+230
-17
No files matched your search
@@ -12,6 +12,7 @@ import (
|
||||
"testing"
|
||||
|
||||
"github.com/nextlevelbuilder/goclaw/internal/providers"
|
||||
"github.com/nextlevelbuilder/goclaw/internal/tools"
|
||||
)
|
||||
|
||||
// imageCapableProvider is a stub provider that also implements CapabilitiesAware
|
||||
@@ -115,3 +116,61 @@ func TestImageGenGate_FinalIteration_AllToolsStripped(t *testing.T) {
|
||||
t.Errorf("final iteration must strip all tools; got %d: %v", len(defs), defs)
|
||||
}
|
||||
}
|
||||
|
||||
type filteringExecutor struct {
|
||||
stubExecutor
|
||||
defs []providers.ToolDefinition
|
||||
}
|
||||
|
||||
func (e *filteringExecutor) ProviderDefs() []providers.ToolDefinition {
|
||||
return e.defs
|
||||
}
|
||||
|
||||
func (e *filteringExecutor) Get(name string) (tools.Tool, bool) {
|
||||
return nil, false
|
||||
}
|
||||
|
||||
func TestImageGenGate_FilteringNoPanic(t *testing.T) {
|
||||
prov := &imageCapableProvider{imageGen: true}
|
||||
|
||||
exec := &filteringExecutor{
|
||||
defs: []providers.ToolDefinition{
|
||||
{
|
||||
Type: "function",
|
||||
Function: &providers.ToolFunctionSchema{
|
||||
Name: "read_file",
|
||||
},
|
||||
},
|
||||
{
|
||||
Type: "image_generation",
|
||||
Function: nil,
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
l := &Loop{
|
||||
provider: prov,
|
||||
allowImageGeneration: true,
|
||||
tools: exec,
|
||||
orchMode: "spawn", // Triggers orchModeDenyTools
|
||||
disabledTools: map[string]bool{"read_file": true}, // Triggers disabled tools filter
|
||||
agentType: "open", // Triggers bootstrap filter
|
||||
skillEvolve: false, // Triggers skill evolve filter
|
||||
}
|
||||
|
||||
req := &RunRequest{
|
||||
ChannelType: "telegram", // Triggers channel filtering
|
||||
}
|
||||
|
||||
// This should run successfully without panic.
|
||||
defs, allowed, _ := l.buildFilteredTools(req, true, 1, 10, nil, nil)
|
||||
|
||||
if allowed != nil {
|
||||
if _, exists := allowed[""]; exists {
|
||||
t.Error("allowedTools map should not contain empty key for native tools")
|
||||
}
|
||||
}
|
||||
|
||||
_ = defs
|
||||
}
|
||||
|
||||
@@ -31,9 +31,11 @@ func (l *Loop) filteredToolNames() []string {
|
||||
return l.tools.List()
|
||||
}
|
||||
defs := l.toolPolicy.FilterTools(l.tools, l.id, l.provider.Name(), l.agentToolPolicy, nil, false, false)
|
||||
names := make([]string, len(defs))
|
||||
for i, d := range defs {
|
||||
names[i] = d.Function.Name
|
||||
names := make([]string, 0, len(defs))
|
||||
for _, d := range defs {
|
||||
if d.Function != nil {
|
||||
names = append(names, d.Function.Name)
|
||||
}
|
||||
}
|
||||
return names
|
||||
}
|
||||
|
||||
@@ -239,7 +239,7 @@ func (l *Loop) makeBuildFilteredTools(req *RunRequest) func(state *pipeline.RunS
|
||||
}
|
||||
mcpDefs := 0
|
||||
for _, td := range toolDefs {
|
||||
if strings.HasPrefix(strings.TrimSpace(td.Function.Name), "mcp_") {
|
||||
if td.Function != nil && strings.HasPrefix(strings.TrimSpace(td.Function.Name), "mcp_") {
|
||||
mcpDefs++
|
||||
}
|
||||
}
|
||||
@@ -508,6 +508,9 @@ func (l *Loop) makeCallLLM(req *RunRequest, emitRun func(AgentEvent)) func(ctx c
|
||||
func shouldRetryTaskMCP(chatReq providers.ChatRequest) bool {
|
||||
hasTaskMCPTool := false
|
||||
for _, td := range chatReq.Tools {
|
||||
if td.Function == nil {
|
||||
continue
|
||||
}
|
||||
name := strings.TrimSpace(td.Function.Name)
|
||||
if strings.HasPrefix(name, "mcp_bx24__") && (strings.Contains(name, "search") || strings.Contains(name, "execute")) {
|
||||
hasTaskMCPTool = true
|
||||
|
||||
@@ -39,7 +39,9 @@ func (l *Loop) buildFilteredTools(req *RunRequest, hadBootstrap bool, iteration,
|
||||
toolDefs = l.toolPolicy.FilterTools(registry, l.id, l.provider.Name(), l.agentToolPolicy, req.ToolAllow, false, false)
|
||||
allowedTools = make(map[string]bool, len(toolDefs))
|
||||
for _, td := range toolDefs {
|
||||
allowedTools[td.Function.Name] = true
|
||||
if td.Function != nil {
|
||||
allowedTools[td.Function.Name] = true
|
||||
}
|
||||
}
|
||||
} else {
|
||||
// No policy → all tools allowed. ProviderDefs() omits per-user MCP tools (not in
|
||||
@@ -67,10 +69,12 @@ func (l *Loop) buildFilteredTools(req *RunRequest, hadBootstrap bool, iteration,
|
||||
if orchDeny := orchModeDenyTools(l.orchMode); len(orchDeny) > 0 {
|
||||
filtered := toolDefs[:0:0]
|
||||
for _, td := range toolDefs {
|
||||
if !orchDeny[td.Function.Name] {
|
||||
if td.Function == nil || !orchDeny[td.Function.Name] {
|
||||
filtered = append(filtered, td)
|
||||
} else {
|
||||
delete(allowedTools, td.Function.Name)
|
||||
if allowedTools != nil {
|
||||
delete(allowedTools, td.Function.Name)
|
||||
}
|
||||
}
|
||||
}
|
||||
toolDefs = filtered
|
||||
@@ -80,10 +84,12 @@ func (l *Loop) buildFilteredTools(req *RunRequest, hadBootstrap bool, iteration,
|
||||
if len(l.disabledTools) > 0 {
|
||||
filtered := toolDefs[:0]
|
||||
for _, td := range toolDefs {
|
||||
if !l.disabledTools[td.Function.Name] {
|
||||
if td.Function == nil || !l.disabledTools[td.Function.Name] {
|
||||
filtered = append(filtered, td)
|
||||
} else {
|
||||
delete(allowedTools, td.Function.Name)
|
||||
if allowedTools != nil {
|
||||
delete(allowedTools, td.Function.Name)
|
||||
}
|
||||
}
|
||||
}
|
||||
toolDefs = filtered
|
||||
@@ -94,7 +100,7 @@ func (l *Loop) buildFilteredTools(req *RunRequest, hadBootstrap bool, iteration,
|
||||
if hadBootstrap && l.agentType != store.AgentTypePredefined {
|
||||
var bootstrapDefs []providers.ToolDefinition
|
||||
for _, td := range toolDefs {
|
||||
if bootstrapToolAllowlist[td.Function.Name] {
|
||||
if td.Function != nil && bootstrapToolAllowlist[td.Function.Name] {
|
||||
bootstrapDefs = append(bootstrapDefs, td)
|
||||
}
|
||||
}
|
||||
@@ -106,7 +112,7 @@ func (l *Loop) buildFilteredTools(req *RunRequest, hadBootstrap bool, iteration,
|
||||
if !l.skillEvolve {
|
||||
filtered := toolDefs[:0:0]
|
||||
for _, td := range toolDefs {
|
||||
if td.Function.Name != "skill_manage" {
|
||||
if td.Function == nil || td.Function.Name != "skill_manage" {
|
||||
filtered = append(filtered, td)
|
||||
}
|
||||
}
|
||||
@@ -117,10 +123,12 @@ func (l *Loop) buildFilteredTools(req *RunRequest, hadBootstrap bool, iteration,
|
||||
if req.ChannelType != "" {
|
||||
filtered := toolDefs[:0:0]
|
||||
for _, td := range toolDefs {
|
||||
if tool, ok := l.tools.Get(td.Function.Name); ok {
|
||||
if ca, ok := tool.(tools.ChannelAware); ok {
|
||||
if !slices.Contains(ca.RequiredChannelTypes(), req.ChannelType) {
|
||||
continue
|
||||
if td.Function != nil {
|
||||
if tool, ok := l.tools.Get(td.Function.Name); ok {
|
||||
if ca, ok := tool.(tools.ChannelAware); ok {
|
||||
if !slices.Contains(ca.RequiredChannelTypes(), req.ChannelType) {
|
||||
continue
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3183,3 +3183,51 @@ func TestPruneStage_CacheTtlGate_MarkTouchedOnlyOnMutation(t *testing.T) {
|
||||
t.Error("MarkCacheTouched should NOT be called when prune returns no mutation")
|
||||
}
|
||||
}
|
||||
|
||||
func TestThinkStage_AllowedTools_MixedFunctionAndNativeTools_NoPanic(t *testing.T) {
|
||||
t.Parallel()
|
||||
deps := &PipelineDeps{
|
||||
Config: PipelineConfig{MaxIterations: 10, MaxTokens: 1000},
|
||||
BuildFilteredTools: func(state *RunState) ([]providers.ToolDefinition, error) {
|
||||
return []providers.ToolDefinition{
|
||||
{
|
||||
Type: "function",
|
||||
Function: &providers.ToolFunctionSchema{
|
||||
Name: "read_file",
|
||||
},
|
||||
},
|
||||
{
|
||||
Type: "image_generation",
|
||||
Function: nil,
|
||||
},
|
||||
}, nil
|
||||
},
|
||||
CallLLM: func(_ context.Context, _ *RunState, _ providers.ChatRequest) (*providers.ChatResponse, error) {
|
||||
return &providers.ChatResponse{
|
||||
Content: "ok",
|
||||
FinishReason: "stop",
|
||||
}, nil
|
||||
},
|
||||
}
|
||||
stage := NewThinkStage(deps)
|
||||
state := defaultState()
|
||||
|
||||
err := stage.Execute(context.Background(), state)
|
||||
if err != nil {
|
||||
t.Fatalf("Execute() error: %v", err)
|
||||
}
|
||||
|
||||
if state.Tool.AllowedTools == nil {
|
||||
t.Fatal("AllowedTools should not be nil")
|
||||
}
|
||||
if !state.Tool.AllowedTools["read_file"] {
|
||||
t.Error("expected read_file in AllowedTools")
|
||||
}
|
||||
if state.Tool.AllowedTools[""] {
|
||||
t.Error("AllowedTools should not contain empty key for native tools")
|
||||
}
|
||||
if len(state.Tool.AllowedTools) != 1 {
|
||||
t.Errorf("AllowedTools len = %d, want 1", len(state.Tool.AllowedTools))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -44,7 +44,9 @@ func (s *ThinkStage) Execute(ctx context.Context, state *RunState) error {
|
||||
}
|
||||
allowed := make(map[string]bool, len(toolDefs))
|
||||
for _, td := range toolDefs {
|
||||
allowed[td.Function.Name] = true
|
||||
if td.Function != nil {
|
||||
allowed[td.Function.Name] = true
|
||||
}
|
||||
}
|
||||
state.Tool.AllowedTools = allowed
|
||||
} else {
|
||||
|
||||
@@ -324,3 +324,51 @@ func assertHeader(t *testing.T, h http.Header, key, want string) {
|
||||
t.Errorf("header %q = %q, want %q", key, got, want)
|
||||
}
|
||||
}
|
||||
|
||||
func TestAnthropicAdapterToRequest_NativeToolIgnored(t *testing.T) {
|
||||
adapter, _ := NewAnthropicAdapter(ProviderConfig{APIKey: "sk-test"})
|
||||
|
||||
req := ChatRequest{
|
||||
Messages: []Message{{Role: "user", Content: "Draw a cat and search"}},
|
||||
Tools: []ToolDefinition{
|
||||
{
|
||||
Type: "function",
|
||||
Function: &ToolFunctionSchema{
|
||||
Name: "web_search",
|
||||
Description: "Search the web",
|
||||
Parameters: map[string]any{"type": "object"},
|
||||
},
|
||||
},
|
||||
{
|
||||
Type: "image_generation",
|
||||
Function: nil,
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
data, _, err := adapter.ToRequest(req)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
var body map[string]any
|
||||
if err := json.Unmarshal(data, &body); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
tools, ok := body["tools"].([]any)
|
||||
if !ok {
|
||||
t.Fatal("expected tools array in request body")
|
||||
}
|
||||
|
||||
// Should only serialize function tool, ignoring native tool
|
||||
if len(tools) != 1 {
|
||||
t.Fatalf("expected 1 tool in Anthropic request, got %d", len(tools))
|
||||
}
|
||||
|
||||
tool := tools[0].(map[string]any)
|
||||
if tool["name"] != "web_search" {
|
||||
t.Errorf("expected tool name 'web_search', got %v", tool["name"])
|
||||
}
|
||||
}
|
||||
|
||||
@@ -195,6 +195,9 @@ func (p *AnthropicProvider) buildRequestBody(model string, req ChatRequest, stre
|
||||
if len(req.Tools) > 0 {
|
||||
var tools []map[string]any
|
||||
for _, t := range req.Tools {
|
||||
if t.Type != "function" || t.Function == nil {
|
||||
continue
|
||||
}
|
||||
cleanedParams := CleanSchemaForProvider("anthropic", t.Function.Parameters)
|
||||
tool := map[string]any{
|
||||
"name": t.Function.Name,
|
||||
|
||||
@@ -122,7 +122,7 @@ func (p *CodexProvider) buildRequestBody(req ChatRequest, stream bool) map[strin
|
||||
"output_format": "png",
|
||||
"partial_images": 1,
|
||||
})
|
||||
} else {
|
||||
} else if t.Function != nil {
|
||||
// Function tool path (default). Works with both value-type and pointer Function
|
||||
// fields — we only access t.Function when type is not "image_generation".
|
||||
tools = append(tools, map[string]any{
|
||||
|
||||
@@ -1083,3 +1083,43 @@ func TestCodexProviderBuildRequestBodyWithImages(t *testing.T) {
|
||||
t.Errorf("content[1] type = %v, want input_text", content[1]["type"])
|
||||
}
|
||||
}
|
||||
|
||||
func TestCodexBuildRequestBody_NilFunction_HandlesGracefully(t *testing.T) {
|
||||
p := NewCodexProvider("test", &staticTokenSource{token: "test"}, "", "gpt-4o")
|
||||
|
||||
req := ChatRequest{
|
||||
Messages: []Message{{Role: "user", Content: "Draw and search"}},
|
||||
Tools: []ToolDefinition{
|
||||
{
|
||||
Type: "some_unsupported_native_tool",
|
||||
Function: nil,
|
||||
},
|
||||
{
|
||||
Type: "function",
|
||||
Function: &ToolFunctionSchema{
|
||||
Name: "web_search",
|
||||
Description: "Search the web",
|
||||
Parameters: map[string]any{"type": "object"},
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
body := p.buildRequestBody(req, false)
|
||||
|
||||
tools, ok := body["tools"].([]map[string]any)
|
||||
if !ok {
|
||||
t.Fatalf("tools is not []map[string]any: %T", body["tools"])
|
||||
}
|
||||
|
||||
// Should only serialize the function tool and ignore the native tool with nil Function
|
||||
if len(tools) != 1 {
|
||||
t.Fatalf("expected 1 tool in Codex request, got %d", len(tools))
|
||||
}
|
||||
|
||||
tool := tools[0]
|
||||
if tool["type"] != "function" || tool["name"] != "web_search" {
|
||||
t.Errorf("expected function tool 'web_search', got: %v", tool)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user