feat(llm): exercise a tool-call round trip in ocr llm test

[Moon-Wrecker]

Description

ocr llm test sends one request with no tools. That never covers the turn that follows a tool call, which is where an OpenAI-compatible provider carrying its own tool-call metadata rejects the conversation.

That gap is what made #1357 confusing to report. On Vertex AI with google/gemini-3.8-flash the test said:

✓ Connection test successful

while every single review failed at the first file_read. The configuration was fine; the request shape the test used was simply not the one that breaks.

This makes the connectivity test exercise the same shape a review does: offer a trivial tool, and when the model calls it, send the result back as a second turn.

• LlmConversation gains an optional tool, so the task config decides what is offered and what canned result comes back. A task without one behaves exactly as before.
• task.json defines ocr_selftest, which takes a short string and returns a fixed acknowledgement.
• runLLMTest offers the tool, and on a call replays it through NewToolCallMessage + NewToolResultMessage for the second request.

When the model does not call the tool, the test reports the round trip as unverified rather than passing silently. A model declining the tool is not evidence that the provider handles the turn, and quietly treating it as success would rebuild the blind spot this is meant to remove.

This is independent of the extra_content fix in #1393 and does not depend on it. It is the detection half: #1393 stops the failure, this stops it from hiding.

The pattern is not specific to Gemini. #156 was a misleading connectivity result from the same command, fixed in #162 by adding the explicit (empty response) and ✓ Connection test successful lines this builds on. #571 reported llm test passing while ocr scan returned 401. #700 reports a provider rejecting cache_control with a 400 on every review, and notes in passing: "BTW: ocr llm test was successful". None of those are fixed by this change — each has a different trigger, and a tool round trip would not catch them — but they are three separate reports of the probe clearing while real work fails, which is the shape this narrows.

How Has This Been Tested?

• Manual testing (below)
• make test — not run in full; see note

go test passes on internal/config/testconnection and cmd/opencodereview, with go vet and gofmt -s clean. I have not run the whole suite to completion on this machine — TestRangeDiffDetectsRename in internal/diff fails here for me with an unmodified tree as well, caused by a stray directory in my checkout — so I have not claimed a pass I did not see.

Live against Vertex AI (aiplatform.googleapis.com/.../endpoints/openapi/chat/completions, google/gemini-3.8-flash), which is the endpoint from #1357. This branch is cut from main and deliberately does not contain the #1393 fix, so it shows the detection working on a genuinely broken setup:

build	ocr llm test
v1.12.5 release binary	✓ Connection test successful — the false pass
this branch, without #1393	Error: llm request after tool call failed: POST ".../chat/completions": 400 Bad Request
this branch, with #1393	✓ Connection test successful + ✓ Tool-call round trip verified

In the passing run the model's answer quotes the tool's return value back — "With connectivity confirmed (ocr_selftest ok), I am open-code-review…" — so the second turn really did carry the tool result rather than the model inventing an answer.

Unit tests cover the reporting contract (verified / unverified / nothing offered, and that the unverified line carries no success mark), the mapping from the configured tool onto llm.ToolDef, a task that defines no tool, and picking the self-test call out of a response that also contains other tool calls.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

Behaviour change worth calling out: ocr llm test now makes up to two requests instead of one, and can now fail where it previously passed. That is the intent — it fails only where a review would also fail.

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes (see the note above on the full suite)
• I have updated the documentation accordingly (if applicable) — the command's own output is the documentation here
• I have signed the CLA
• I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

AI/LLM disclosure: I used Claude (Opus 5) to help refine this change. I reviewed and understand every line, ran the live Vertex verification myself, and will answer review comments myself.

Related Issues

Refs #1357

[github-actions]

🔍 OpenCodeReview found 2 issue(s) in this PR.

• ✅ Successfully posted inline: 2 comment(s)

[chaojixinren]

The approach makes sense. Please address the multi-call handling issue noted inline and add a regression test before merging. Otherwise, make test and make check both pass locally.

[chaojixinren]

This replays all tool calls but only returns a result for the first one. If the model returns multiple calls, a working provider may reject the next request.

Could you return a result for every call and add a regression test?

[Moon-Wrecker]

Thanks you were right, and it was worse than one missed result. I reproduced the old construction against a response with three calls: it replayed all three and answered one, leaving call_2 and call_3 unanswered. Both supported families reject that, so a perfectly healthy provider would have returned a 400 on the second request indistinguishable from the provider rejection this command exists to detect. The command would have blamed the endpoint for its own malformed request.

Fixed in toolResultMessages, which answers every call in the replayed turn:

• a call to the configured self-test tool gets the configured result, including a repeated call under a different id;
• any other call gets Error: this tool is not available in ocr llm test and was not executed. an honest non-execution rather than an invented outcome, and non-empty because some providers reject an empty result.

I considered the narrower alternative of replaying only the self-test call so there would be exactly one call and one result, and rejected it for two reasons. It sends a turn the model did not produce; and it would not reliably work anyway, because buildAnthropicParams reuses the whole native assistant message (internal/llm/client.go:1364), so a filtered slice would not be what goes on the wire. Replaying the real turn and answering all of it is both more faithful and more portable.

Regression test as requested: TestSecondTurnPairsEveryToolCall builds the second request exactly as runLLMTest does and asserts every replayed tool call is answered exactly once, with a mix of offered, unknown and repeated calls. TestToolResultMessages_AnswersEveryCall covers the mapping directly.

Two related notes while I was in here:

• The Anthropic path hardcodes is_error=false when building tool-result blocks (client.go:1343), so the unknown-tool result above cannot be marked as an error on that path. Only the text conveys it. Fixing that means changing the shared result helper, which felt out of scope for this PR happy to follow up separately if you would like it.
• Re-verified live against Vertex with google/gemini-3.8-flash after the change: on this branch without fix(llm): replay tool_call extra_content and surface provider error bodies #1393 the command still correctly fails at the post-tool-call turn, which is the behaviour the PR is for.

make test and make check equivalents pass here too, with the caveat already in the description that I have not run the full suite to completion on this machine.

[Moon-Wrecker]

One further question while looking at this, which I would rather ask than decide unilaterally.

The round trip is still optional. The first request does not force tool choice, so a model that ignores the prompt produces no tool call, and the command prints ✓ Connection test successful, adds the unverified note, and exits zero. That is honest, but it means the check is best-effort: on a genuinely broken provider a model that simply declines the tool would let the setup pass.

Setting ToolChoice: "required" on the first request only, leaving the second at the provider default, would make it deterministic. All three adapters already support it.

The reason I have not done it is the Anthropic trade-off: internal/llm/client.go:1305-1308 deliberately drops thinking from extra_body when tool choice is forced. So forcing it would make the probe stop exercising the thinking-signature replay path for Anthropic users who configure extended thinking — the very path #1070 added. The cost only lands on configs that set extra_body.thinking, and ocr llm test is aimed at connectivity rather than thinking coverage, so my inclination is that determinism is worth more here. But it is a behaviour call on the command's purpose, and it is beyond what you asked for, so I would rather have your read.

Happy to add it to this PR, keep it as a follow-up, or leave it as-is.

[chaojixinren]

One further question while looking at this, which I would rather ask than decide unilaterally.

The round trip is still optional. The first request does not force tool choice, so a model that ignores the prompt produces no tool call, and the command prints ✓ Connection test successful, adds the unverified note, and exits zero. That is honest, but it means the check is best-effort: on a genuinely broken provider a model that simply declines the tool would let the setup pass.

Setting ToolChoice: "required" on the first request only, leaving the second at the provider default, would make it deterministic. All three adapters already support it.

The reason I have not done it is the Anthropic trade-off: internal/llm/client.go:1305-1308 deliberately drops thinking from extra_body when tool choice is forced. So forcing it would make the probe stop exercising the thinking-signature replay path for Anthropic users who configure extended thinking — the very path #1070 added. The cost only lands on configs that set extra_body.thinking, and ocr llm test is aimed at connectivity rather than thinking coverage, so my inclination is that determinism is worth more here. But it is a behaviour call on the command's purpose, and it is beyond what you asked for, so I would rather have your read.

Happy to add it to this PR, keep it as a follow-up, or leave it as-is.

Thanks, the multi-call fix addresses my concern, and the regression test covers the mixed and repeated calls.

On forced tool choice, let's keep the current behavior in this PR and discuss stricter verification separately. Preserving the configured Anthropic thinking/signature replay path is valuable, and the explicit unverified message makes the current limitation clear. We can also handle is_error support separately.

[Moon-Wrecker]

Understood, thanks — leaving forced tool choice out of this PR and keeping the unverified note as the stated limitation. I agree the configured Anthropic thinking/signature replay path is the more valuable thing to protect here; that was the side of the trade-off I was least sure about.

Happy to open follow-ups for stricter verification and for is_error on the Anthropic tool-result path whenever you want them, or to leave both until after this lands.

Nothing further from me on this PR.

[Moon-Wrecker]

@NanaseInori review

[lizhengfeng101]

LGTM