fix(llmloop): guard nil tool-call arguments map to prevent panic

[chethanuk]

Problem

ocr scan (and potentially review) crashes with panic: assignment to entry in nil map at internal/llmloop/loop.go in executeToolCall, killing the per-file subtask:

panic: assignment to entry in nil map
github.com/open-code-review/open-code-review/internal/llmloop.(*Runner).executeToolCall(...)
    internal/llmloop/loop.go:324
github.com/open-code-review/open-code-review/internal/llmloop.(*Runner).RunPerFile(...)
    internal/scan.(*Agent).executeSubtask → dispatchBatch

Reported with GLM via an OpenAI-compatible endpoint; reproduces at any --concurrency, but only on files where the model emits the offending tool call — matching the reporter's partial-failure sessions ("5 completed, 4 failed").

Root cause

Some OpenAI-compatible gateways emit "arguments": null for tool calls. json.Unmarshal([]byte("null"), &args) returns nil error and leaves the map nil. The code_comment path-override then writes into it:

var args map[string]any
if err := json.Unmarshal([]byte(call.Function.Arguments), &args); err != nil { ... } // "null" passes
...
if t == tool.CodeComment && newPath != "" {
    args["path"] = newPath // panic: assignment to entry in nil map
}

This is the only nil-map write in the tool-call handling surface (audited every Function.Arguments unmarshal in the repo), but the dynamic-tool path has the same latent hazard: providers receive a nil args map and would panic if they ever write to it.

Fix

1. internal/llmloop/loop.go — both argument-unmarshal sites in executeToolCall now go through a shared parseToolArgs helper that always returns a non-nil map, so every downstream consumer (ParseComments, provider Execute, the code_comment path override) is safe. A null-args code_comment call now degrades gracefully to the existing "'comments' array is required" tool-error result, which the model can react to — instead of killing the subtask.
2. internal/llm/client.go — the Anthropic history-replay path had the same latent hazard: argsMap is pre-initialized, but json.Unmarshal([]byte("null"), &argsMap) resets it to nil (JSON null sets maps to nil regardless of prior value), so tool_use input would serialize as JSON null, which the API rejects. Guarded the same way.

Intentional behavior note: for dynamic/MCP tools called with arguments: null, the wire payload to the MCP server changes from "arguments": null to "arguments": {} — the standards-conformant encoding (JSON-Schema type: object validators reject null).

Tests

• New table-driven TestExecuteToolCall_ArgumentsEdgeCases (internal/llmloop): null args on code_comment (the exact panic: assignment to entry in nil map #382 repro — panics before this fix), {}, valid args (path-override behavior preserved), empty string, malformed JSON, and null args on a dynamic tool (asserts Execute receives a non-nil map).
• New table-driven TestBuildAnthropicParams_NullToolCallArguments (internal/llm): null/empty/{} arguments → tool_use input is always a non-nil object (fails on the nil map before the fix).

Both tests verified RED before the fix, GREEN after:

$ go test ./internal/llmloop/ ./internal/llm/ -race -count=1
ok  (325 tests, 2 packages)

make check, make test (-race), make coverage (81%+, ≥80% gate) and go build ./cmd/opencodereview all pass.

Fixes #382

[github-actions]

✅ OpenCodeReview: No comments generated. Looks good to me.

[lizhengfeng101]

Review

Solid bug fix — root cause analysis is accurate, the fix is minimal and defensive, and test coverage is thorough. A few minor suggestions below, none blocking.

1. Consider unifying the nil-guard logic across packages

loop.go extracts parseToolArgs as a helper, but client.go uses an inline if argsMap == nil check. Since they're in different packages, direct reuse isn't trivial, but it's worth either:

• Promoting parseToolArgs to a shared location (e.g. internal/llm), or
• At minimum, cross-referencing the two sites in comments so future maintainers know both exist.

Not a blocker — just a maintainability note.

2. Trim the comment in client.go:689-693

The three-line comment says one thing in three ways. Suggest condensing:

// null arguments → empty map; Anthropic API rejects null input (#382).
argsMap = map[string]any{}

3. Tests are excellent

The table-driven cases cover the exact panic repro (null args on code_comment), normal path-override behavior, malformed JSON, empty strings, and the dynamic-tool nil-args path. argsCapturingProvider is a clean, focused mock. Nothing missing here.

Summary

Low-risk, well-tested defensive fix. The graceful degradation (null args → existing "'comments' array is required" error instead of panic) is the right call. LGTM — approve after the optional comment cleanup.

[chethanuk]

Thanks for the review @lizhengfeng101! Both comment nits addressed in bb3928c: trimmed the client.go comment to your suggested wording (plus a pointer to llmloop.parseToolArgs) and added the reverse pointer in the parseToolArgs doc comment. Took the cross-reference option over promoting a shared helper — the client.go guard sits inside a conditional-unmarshal branch with a pre-initialized map and its own error wrapping, so a cross-package export would serve exactly one caller and change error semantics for the other site. Happy to revisit if a third nil-args site ever appears.

[css521]

LGTM

[lizhengfeng101]

LGTM