From 850c99d4b340bc1d5f062430ab75d07e27148894 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Fri, 17 Jul 2026 11:45:49 +0200 Subject: [PATCH 1/2] fix(llmloop): guard nil tool-call arguments map to prevent panic Some OpenAI-compatible gateways emit "arguments": null for tool calls. json.Unmarshal("null", &args) succeeds and sets the map to nil (JSON null nils maps regardless of prior value), so the code_comment path override (args["path"] = newPath) panicked with "assignment to entry in nil map", killing the per-file subtask. - internal/llmloop: parse arguments through a shared parseToolArgs helper that always returns a non-nil map, covering both the known-tool and dynamic-tool paths. - internal/llm: the Anthropic history-replay path had the same hazard -- null arguments reset the pre-initialized argsMap to nil, serializing tool_use input as JSON null, which the API rejects. Fixes #382 --- internal/llm/client.go | 6 ++ internal/llm/client_test.go | 62 +++++++++++++++++++ internal/llmloop/loop.go | 23 +++++-- internal/llmloop/loop_test.go | 112 ++++++++++++++++++++++++++++++++++ 4 files changed, 199 insertions(+), 4 deletions(-) diff --git a/internal/llm/client.go b/internal/llm/client.go index 25b7266e..001a4345 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -686,6 +686,12 @@ func (c *AnthropicClient) buildAnthropicParams(model string, req ChatRequest) (a if err := json.Unmarshal([]byte(tc.Function.Arguments), &argsMap); err != nil { return anthropic.MessageNewParams{}, fmt.Errorf("invalid tool call arguments for %s: %w", tc.Function.Name, err) } + if argsMap == nil { + // "arguments": null resets the map to nil; the + // Anthropic API requires tool_use input to be an + // object, not null (#382). + argsMap = map[string]any{} + } } blocks = append(blocks, anthropic.NewToolUseBlock(tc.ID, argsMap, tc.Function.Name)) } diff --git a/internal/llm/client_test.go b/internal/llm/client_test.go index cc490e33..354219a5 100644 --- a/internal/llm/client_test.go +++ b/internal/llm/client_test.go @@ -222,6 +222,68 @@ func TestBuildAnthropicParams_CacheControl_NoSystem(t *testing.T) { } } +func TestBuildAnthropicParams_NullToolCallArguments(t *testing.T) { + // "arguments": null (as emitted by some OpenAI-compatible gateways) + // unmarshals a pre-initialized map back to nil; the Anthropic API + // requires tool_use input to be an object, not null (#382). + client := NewAnthropicClient(ClientConfig{URL: "https://api.anthropic.com"}) + + tests := []struct { + name string + arguments string + }{ + {name: "null arguments", arguments: `null`}, + {name: "empty arguments", arguments: ``}, + {name: "empty object", arguments: `{}`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := ChatRequest{ + Messages: []Message{ + {Role: "user", Content: "Hello"}, + { + Role: "assistant", + ToolCalls: []ToolCall{{ + ID: "call_1", + Type: "function", + Function: FunctionCall{ + Name: "code_comment", + Arguments: tt.arguments, + }, + }}, + }, + }, + } + + params, err := client.buildAnthropicParams("claude-sonnet-4-20250514", req) + if err != nil { + t.Fatalf("buildAnthropicParams: %v", err) + } + + var found bool + for _, m := range params.Messages { + for _, b := range m.Content { + if b.OfToolUse == nil { + continue + } + found = true + input, ok := b.OfToolUse.Input.(map[string]any) + if !ok { + t.Fatalf("tool_use input type = %T, want map[string]any", b.OfToolUse.Input) + } + if input == nil { + t.Error("tool_use input is a nil map; API requires an object") + } + } + } + if !found { + t.Fatal("no tool_use block found in built params") + } + }) + } +} + func TestAnthropicClient_UsesConfiguredXAPIKeyHeader(t *testing.T) { t.Setenv("ANTHROPIC_API_KEY", "") t.Setenv("ANTHROPIC_AUTH_TOKEN", "env-oauth-token") diff --git a/internal/llmloop/loop.go b/internal/llmloop/loop.go index c93ce609..b17aef7d 100644 --- a/internal/llmloop/loop.go +++ b/internal/llmloop/loop.go @@ -276,8 +276,8 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T return tool.Of(tool.NotAvailableMsg) } r.recordToolCall(call.Function.Name) - var dynArgs map[string]any - if err := json.Unmarshal([]byte(call.Function.Arguments), &dynArgs); err != nil { + dynArgs, err := parseToolArgs(call.Function.Arguments) + if err != nil { return tool.Of(fmt.Sprintf("Error parsing tool arguments for %s: %v", call.Function.Name, err)) } telemetry.PrintToolCallStarted(call.Function.Name, dynArgs) @@ -313,8 +313,8 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T r.recordToolCall(t.Name()) - var args map[string]any - if err := json.Unmarshal([]byte(call.Function.Arguments), &args); err != nil { + args, err := parseToolArgs(call.Function.Arguments) + if err != nil { return tool.Of(fmt.Sprintf("Error parsing tool arguments for %s: %v", t.Name(), err)) } @@ -468,6 +468,21 @@ func (r *Runner) addNextMessage(ctx context.Context, assistantContent string, to return CountMessagesTokens(*messages) < warnLimit } +// parseToolArgs unmarshals a tool call's raw JSON arguments, always +// returning a non-nil map on success: some OpenAI-compatible gateways send +// "arguments": null, which unmarshals to a nil map and would panic on the +// first write (#382). +func parseToolArgs(raw string) (map[string]any, error) { + var args map[string]any + if err := json.Unmarshal([]byte(raw), &args); err != nil { + return nil, err + } + if args == nil { + args = make(map[string]any) + } + return args, nil +} + // lookupTool returns the provider for a given tool from the registry, or // nil when not registered. func lookupTool(reg *tool.Registry, t tool.Tool) tool.Provider { diff --git a/internal/llmloop/loop_test.go b/internal/llmloop/loop_test.go index 352fc208..b2dd9e68 100644 --- a/internal/llmloop/loop_test.go +++ b/internal/llmloop/loop_test.go @@ -3,6 +3,7 @@ package llmloop import ( "context" "encoding/json" + "strings" "testing" "github.com/open-code-review/open-code-review/internal/config/template" @@ -269,6 +270,117 @@ func TestRunner_RecordUsage(t *testing.T) { } } +// argsCapturingProvider records the args map Execute receives, so tests can +// assert the runner never hands tools a nil map. +type argsCapturingProvider struct { + tool tool.Tool + gotArgs map[string]any + captured bool +} + +func (p *argsCapturingProvider) Tool() tool.Tool { return p.tool } +func (p *argsCapturingProvider) Execute(_ context.Context, args map[string]any) (string, error) { + p.gotArgs = args + p.captured = true + return "ok", nil +} + +func TestExecuteToolCall_ArgumentsEdgeCases(t *testing.T) { + // Regression for #382: some OpenAI-compatible gateways emit + // "arguments": null; json.Unmarshal("null", &m) leaves m nil, and the + // code_comment path override then panicked with "assignment to entry + // in nil map". + tests := []struct { + name string + toolName string + arguments string + wantContains string // substring expected in cp.Data ("" = skip) + wantComment string // if non-empty, expect one collected comment with this path + wantNonNilArgs bool // dynamic tool: Execute must receive a non-nil args map + }{ + { + name: "null args on code_comment (issue #382)", + toolName: "code_comment", + arguments: `null`, + wantContains: "'comments' array is required", + }, + { + name: "empty object on code_comment", + toolName: "code_comment", + arguments: `{}`, + wantContains: "'comments' array is required", + }, + { + name: "valid args keeps path override", + toolName: "code_comment", + arguments: `{"path":"hallucinated.go","comments":[{"content":"issue","existing_code":"foo"}]}`, + wantComment: "file.go", + }, + { + name: "empty string args", + toolName: "code_comment", + arguments: ``, + wantContains: "Error parsing tool arguments", + }, + { + name: "malformed json args", + toolName: "code_comment", + arguments: `{"comments":`, + wantContains: "Error parsing tool arguments", + }, + { + name: "null args on dynamic tool", + toolName: "dyn_echo", + arguments: `null`, + wantNonNilArgs: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + collector := tool.NewCommentCollector() + dyn := &argsCapturingProvider{tool: tool.Dynamic("dyn_echo")} + reg := tool.NewRegistry() + reg.Register(&tool.CodeCommentProvider{Collector: collector}) + reg.Register(dyn) + reg.Freeze() + + r := NewRunner(Deps{ + Tools: reg, + CommentCollector: collector, + }) + + cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{ + Function: llm.FunctionCall{ + Name: tt.toolName, + Arguments: tt.arguments, + }, + }, nil) + + if tt.wantContains != "" && !strings.Contains(cp.Data, tt.wantContains) { + t.Errorf("cp.Data = %q, want substring %q", cp.Data, tt.wantContains) + } + if tt.wantComment != "" { + comments := collector.Comments() + if len(comments) != 1 { + t.Fatalf("expected 1 comment, got %d", len(comments)) + } + if comments[0].Path != tt.wantComment { + t.Errorf("comment path = %q, want %q", comments[0].Path, tt.wantComment) + } + } + if tt.wantNonNilArgs { + if !dyn.captured { + t.Fatal("dynamic tool Execute was not called") + } + if dyn.gotArgs == nil { + t.Error("dynamic tool Execute received nil args map, want non-nil empty map") + } + } + }) + } +} + func TestExecuteToolCall_CodeCommentOverridesHallucinatedPath(t *testing.T) { collector := tool.NewCommentCollector() reg := tool.NewRegistry() From bb3928c4b388a6e75d67f95fcf0cd47e09e5ac75 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Mon, 20 Jul 2026 12:16:09 +0200 Subject: [PATCH 2/2] docs(llm): trim nil-args comment and cross-reference parseToolArgs Review feedback on #393: the two null-arguments guards now reference each other instead of sharing a helper; a 2-line guard does not justify a cross-package export. --- internal/llm/client.go | 5 ++--- internal/llmloop/loop.go | 3 ++- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/llm/client.go b/internal/llm/client.go index 001a4345..1ecc01e2 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -687,9 +687,8 @@ func (c *AnthropicClient) buildAnthropicParams(model string, req ChatRequest) (a return anthropic.MessageNewParams{}, fmt.Errorf("invalid tool call arguments for %s: %w", tc.Function.Name, err) } if argsMap == nil { - // "arguments": null resets the map to nil; the - // Anthropic API requires tool_use input to be an - // object, not null (#382). + // null arguments → empty map; Anthropic API rejects + // null input (#382). Same guard as llmloop.parseToolArgs. argsMap = map[string]any{} } } diff --git a/internal/llmloop/loop.go b/internal/llmloop/loop.go index b17aef7d..0b435203 100644 --- a/internal/llmloop/loop.go +++ b/internal/llmloop/loop.go @@ -471,7 +471,8 @@ func (r *Runner) addNextMessage(ctx context.Context, assistantContent string, to // parseToolArgs unmarshals a tool call's raw JSON arguments, always // returning a non-nil map on success: some OpenAI-compatible gateways send // "arguments": null, which unmarshals to a nil map and would panic on the -// first write (#382). +// first write (#382). An equivalent inline guard exists in internal/llm's +// buildAnthropicParams; keep the two in sync. func parseToolArgs(raw string) (map[string]any, error) { var args map[string]any if err := json.Unmarshal([]byte(raw), &args); err != nil {