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

cmd/opencodereview/llm_cmd.go

@@ -95,19 +95,44 @@ func runLLMTestWithConfigPath(configPath string) error {
 		messages = append(messages, llm.Message{Role: m.Role, Content: m.Content})
 	}
 
-	resp, err := func() (*llm.ChatResponse, error) {
+	tools := testToolDefs(task.Tool)
+
+	// Each request gets the configured budget, as it did when this test made only
+	// one. A second turn inheriting an exhausted deadline would fail exactly the
+	// way a provider rejecting that turn does, which is the distinction the tool
+	// round trip exists to draw.
+	send := func(msgs []llm.Message) (*llm.ChatResponse, error) {
 		ctx, cancel := context.WithTimeout(context.Background(), timeout)
 		defer cancel()
 		return llmClient.CompletionsWithCtx(ctx, llm.ChatRequest{
 			Model:     ep.Model,
-			Messages:  messages,
+			Messages:  msgs,
+			Tools:     tools,
 			MaxTokens: 2048,
 		})
-	}()
+	}
+
+	resp, err := send(messages)
 	if err != nil {
 		return fmt.Errorf("llm request failed: %w", err)
 	}
 
+	// A single request never exercises the turn that follows a tool call, which
+	// is where providers with their own tool-call metadata reject the
+	// conversation (#1357). Replay the turn so the test covers it, answering
+	// every call the model made rather than only the self-test one.
+	var toolCalled bool
+	if task.Tool != nil {
+		if findTestToolCall(resp, task.Tool.Name) != nil {
+			toolCalled = true
+			messages = append(messages, llm.NewToolCallMessage(resp.VisibleContent(), resp.ToolCalls(), resp.Native(), resp.ReasoningContent()))
+			messages = append(messages, toolResultMessages(resp, task.Tool)...)
+			if resp, err = send(messages); err != nil {
+				return fmt.Errorf("llm request after tool call failed: %w", err)
+			}
+		}
+	}
+
 	model := ep.Model
 	if resp.Model != "" {
 		model = resp.Model
@@ -134,9 +159,80 @@ func runLLMTestWithConfigPath(configPath string) error {
 	}
 	fmt.Printf("%s\n", content)
 	fmt.Println("✓ Connection test successful")
+	if note := toolRoundTripNote(len(tools) > 0, toolCalled); note != "" {
+		fmt.Println(note)
+	}
+	return nil
+}
+
+// testToolDefs offers the configured self-test tool, or none when the task
+// defines no tool.
+func testToolDefs(spec *testconnection.ToolSpec) []llm.ToolDef {
+	if spec == nil || spec.Name == "" {
+		return nil
+	}
+	return []llm.ToolDef{{
+		Type: "function",
+		Function: llm.FunctionDef{
+			Name:        spec.Name,
+			Description: spec.Description,
+			Parameters:  spec.Parameters,
+		},
+	}}
+}
+
+// findTestToolCall returns the model's call to the self-test tool, ignoring any
+// other tool it asked for. An empty name belongs to a task that offers no tool
+// and matches nothing, so an unnamed tool call cannot stand in for one.
+func findTestToolCall(resp *llm.ChatResponse, name string) *llm.ToolCall {
+	if name == "" {
+		return nil
+	}
+	for _, tc := range resp.ToolCalls() {
+		if tc.Function.Name == name {
+			return &tc
+		}
+	}
 	return nil
 }
 
+// unofferedToolResult answers a tool the test never offered. It reports the
+// call as not executed rather than inventing an outcome, and it has to say
+// something: an empty result is itself rejected by some providers.
+const unofferedToolResult = "Error: this tool is not available in ocr llm test and was not executed."
+
+// toolResultMessages answers every tool call in the turn. Both supported
+// protocols reject an assistant turn whose tool calls are not all answered, so
+// leaving one out would fail the next request for a reason unrelated to how the
+// provider handles tool-call metadata — the only thing this test is measuring.
+func toolResultMessages(resp *llm.ChatResponse, spec *testconnection.ToolSpec) []llm.Message {
+	calls := resp.ToolCalls()
+	out := make([]llm.Message, 0, len(calls))
+	for _, tc := range calls {
+		result := unofferedToolResult
+		if spec != nil && tc.Function.Name == spec.Name {
+			result = spec.Result
+		}
+		out = append(out, llm.NewToolResultMessage(tc.ID, result))
+	}
+	return out
+}
+
+// toolRoundTripNote reports what the round trip proved. A provider that never
+// calls the tool leaves it unproven, and saying so is the point: a bare success
+// line is what let an endpoint that rejects every post-tool-call request look
+// healthy here while failing every review (#1357).
+func toolRoundTripNote(offered, called bool) string {
+	switch {
+	case !offered:
+		return ""
+	case called:
+		return "✓ Tool-call round trip verified"
+	default:
+		return "! Tool-call round trip unverified: the model did not call the test tool"
+	}
+}
+
 // bedrockContext reports the region and profile a Bedrock client resolved.
 // ok is false for every other client, which keeps the test output unchanged for
 // URL-based providers.

cmd/opencodereview/llm_cmd_test.go

@@ -5,9 +5,195 @@ package main
 
 import (
 	"path/filepath"
+	"strings"
 	"testing"
+
+	"github.com/alibaba/open-code-review/internal/config/testconnection"
+	"github.com/alibaba/open-code-review/internal/llm"
 )
 
+// TestToolRoundTripNote guards the reporting contract for #1357: a provider that
+// never calls the test tool leaves the round trip unproven, and saying so is the
+// whole point — a plain success line is what let a broken provider look healthy.
+func TestToolRoundTripNote(t *testing.T) {
+	tests := []struct {
+		name    string
+		offered bool
+		called  bool
+		want    string
+	}{
+		{name: "no tool offered says nothing", offered: false, called: false, want: ""},
+		{name: "tool called reports verified", offered: true, called: true, want: "verified"},
+		{name: "tool not called reports unverified", offered: true, called: false, want: "unverified"},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := toolRoundTripNote(tc.offered, tc.called)
+			if tc.want == "" {
+				if got != "" {
+					t.Fatalf("note = %q, want empty", got)
+				}
+				return
+			}
+			if !strings.Contains(got, tc.want) {
+				t.Fatalf("note = %q, want it to contain %q", got, tc.want)
+			}
+		})
+	}
+}
+
+// TestToolRoundTripNote_UnverifiedIsNotMistakenForSuccess keeps the two states
+// textually distinct, so neither reads as the other.
+func TestToolRoundTripNote_UnverifiedIsNotMistakenForSuccess(t *testing.T) {
+	unverified := toolRoundTripNote(true, false)
+	if strings.Contains(unverified, "✓") {
+		t.Errorf("unverified note must not carry a success mark: %q", unverified)
+	}
+}
+
+// TestTestToolDef maps the configured tool onto the shape the LLM clients take.
+func TestTestToolDef(t *testing.T) {
+	spec := &testconnection.ToolSpec{
+		Name:        "ocr_selftest",
+		Description: "echoes",
+		Parameters:  map[string]any{"type": "object"},
+	}
+	tools := testToolDefs(spec)
+	if len(tools) != 1 {
+		t.Fatalf("tools = %d, want 1", len(tools))
+	}
+	if tools[0].Type != "function" {
+		t.Errorf("type = %q, want function", tools[0].Type)
+	}
+	if tools[0].Function.Name != "ocr_selftest" {
+		t.Errorf("name = %q", tools[0].Function.Name)
+	}
+	if tools[0].Function.Parameters["type"] != "object" {
+		t.Errorf("parameters not carried through: %v", tools[0].Function.Parameters)
+	}
+	if got := testToolDefs(nil); got != nil {
+		t.Errorf("a task without a tool must offer none, got %v", got)
+	}
+}
+
+// TestFindTestToolCall picks out the configured tool from a response, ignoring
+// anything else the model may have asked for.
+func TestFindTestToolCall(t *testing.T) {
+	resp := &llm.ChatResponse{Choices: []llm.Choice{{Message: llm.ResponseMessage{ToolCalls: []llm.ToolCall{
+		{ID: "call_other", Function: llm.FunctionCall{Name: "something_else"}},
+		{ID: "call_1", Function: llm.FunctionCall{Name: "ocr_selftest"}},
+	}}}}}
+
+	tc := findTestToolCall(resp, "ocr_selftest")
+	if tc == nil {
+		t.Fatal("expected the test tool call to be found")
+	}
+	if tc.ID != "call_1" {
+		t.Errorf("ID = %q, want call_1", tc.ID)
+	}
+	if findTestToolCall(resp, "absent") != nil {
+		t.Error("expected nil when the tool was not called")
+	}
+	if findTestToolCall(&llm.ChatResponse{}, "ocr_selftest") != nil {
+		t.Error("expected nil for a response with no choices")
+	}
+}
+
+// TestFindTestToolCall_EmptyNameMatchesNothing guards the task-without-a-tool
+// path: an empty name must not match a provider's unnamed tool call, or the
+// caller would go on to dereference a tool spec that does not exist.
+func TestFindTestToolCall_EmptyNameMatchesNothing(t *testing.T) {
+	resp := &llm.ChatResponse{Choices: []llm.Choice{{Message: llm.ResponseMessage{ToolCalls: []llm.ToolCall{
+		{ID: "call_unnamed", Function: llm.FunctionCall{Name: ""}},
+	}}}}}
+
+	if tc := findTestToolCall(resp, ""); tc != nil {
+		t.Fatalf("empty name matched %q; a task with no tool must find nothing", tc.ID)
+	}
+}
+
+// TestToolResultMessages_AnswersEveryCall guards the protocol rule both
+// supported families share: an assistant turn carrying tool calls must be
+// followed by a result for each one. Leaving any unanswered makes the next
+// request fail for a reason that has nothing to do with the provider's handling
+// of tool-call metadata, which is the only thing this test is trying to learn.
+func TestToolResultMessages_AnswersEveryCall(t *testing.T) {
+	spec := &testconnection.ToolSpec{Name: "ocr_selftest", Result: "ocr_selftest ok"}
+	resp := &llm.ChatResponse{Choices: []llm.Choice{{Message: llm.ResponseMessage{ToolCalls: []llm.ToolCall{
+		{ID: "call_1", Function: llm.FunctionCall{Name: "file_read"}},
+		{ID: "call_2", Function: llm.FunctionCall{Name: "ocr_selftest"}},
+		{ID: "call_3", Function: llm.FunctionCall{Name: "code_search"}},
+	}}}}}
+
+	got := toolResultMessages(resp, spec)
+	if len(got) != 3 {
+		t.Fatalf("produced %d results for 3 tool calls, want 3", len(got))
+	}
+	for i, want := range []string{"call_1", "call_2", "call_3"} {
+		if got[i].ToolCallID != want {
+			t.Errorf("result %d answers %q, want %q", i, got[i].ToolCallID, want)
+		}
+		if got[i].Role != "tool" {
+			t.Errorf("result %d role = %q, want tool", i, got[i].Role)
+		}
+	}
+	if got[1].ExtractText() != "ocr_selftest ok" {
+		t.Errorf("self-test call got %q, want the configured result", got[1].ExtractText())
+	}
+	if got[0].ExtractText() == "ocr_selftest ok" {
+		t.Error("a tool the test never offered must not receive the self-test result")
+	}
+	if got[0].ExtractText() == "" {
+		t.Error("every result needs content; an empty one is rejected by some providers")
+	}
+}
+
+// TestToolResultMessages_NoCalls returns nothing when the model called nothing.
+func TestToolResultMessages_NoCalls(t *testing.T) {
+	if got := toolResultMessages(&llm.ChatResponse{}, nil); len(got) != 0 {
+		t.Fatalf("got %d results for a response with no tool calls", len(got))
+	}
+}
+
+// TestSecondTurnPairsEveryToolCall is the regression test for the review on
+// #1394: the second request replays the whole assistant turn, so every tool
+// call in it must be answered exactly once. An unanswered call makes a working
+// provider reject the request, which is indistinguishable from the provider
+// rejection this command exists to detect.
+func TestSecondTurnPairsEveryToolCall(t *testing.T) {
+	spec := &testconnection.ToolSpec{Name: "ocr_selftest", Result: "ocr_selftest ok"}
+	resp := &llm.ChatResponse{Choices: []llm.Choice{{Message: llm.ResponseMessage{ToolCalls: []llm.ToolCall{
+		{ID: "call_1", Function: llm.FunctionCall{Name: "ocr_selftest"}},
+		{ID: "call_2", Function: llm.FunctionCall{Name: "file_read"}},
+		{ID: "call_3", Function: llm.FunctionCall{Name: "ocr_selftest"}},
+	}}}}}
+
+	// The same construction runLLMTest performs for the second request.
+	assistant := llm.NewToolCallMessage(resp.VisibleContent(), resp.ToolCalls(), resp.Native(), resp.ReasoningContent())
+	turn := append([]llm.Message{assistant}, toolResultMessages(resp, spec)...)
+
+	answered := map[string]int{}
+	for _, m := range turn[1:] {
+		answered[m.ToolCallID]++
+	}
+	for _, tc := range assistant.ToolCalls {
+		switch answered[tc.ID] {
+		case 1:
+		case 0:
+			t.Errorf("tool call %q (%s) is replayed but never answered", tc.ID, tc.Function.Name)
+		default:
+			t.Errorf("tool call %q answered %d times, want exactly 1", tc.ID, answered[tc.ID])
+		}
+	}
+	if len(answered) != len(assistant.ToolCalls) {
+		t.Errorf("%d results for %d replayed calls", len(answered), len(assistant.ToolCalls))
+	}
+	// A repeated call to the offered tool is still that tool, not an unknown one.
+	if got := toolResultMessages(resp, spec)[2].ExtractText(); got != "ocr_selftest ok" {
+		t.Errorf("repeated self-test call got %q, want the configured result", got)
+	}
+}
+
 func TestLLMTestCommand_UsesDefaultConfigPath(t *testing.T) {
 	home := t.TempDir()
 	t.Setenv("HOME", home)

internal/config/testconnection/task.json

@@ -3,13 +3,28 @@
     "messages": [
       {
         "role": "system",
-        "content": "## Role\nYou are open-code-review, a code review assistant developed by Alibaba, running in the user's command-line environment.\n\n## Quick Start\nRun `ocr --help` in the command-line environment."
+        "content": "## Role\nYou are open-code-review, a code review assistant developed by Alibaba, running in the user's command-line environment.\n\n## Quick Start\nRun `ocr --help` in the command-line environment.\n\n## Tools\nA tool named `ocr_selftest` is available. Call it once with any short string before you answer, then answer using what it returns."
       },
       {
         "role": "user",
-        "content": "One sentence to answer who you are."
+        "content": "Call the ocr_selftest tool, then answer in one sentence who you are."
       }
     ],
+    "tool": {
+      "name": "ocr_selftest",
+      "description": "Connectivity self-test. Echoes back a fixed acknowledgement so the caller can verify a tool-call round trip.",
+      "parameters": {
+        "type": "object",
+        "properties": {
+          "note": {
+            "type": "string",
+            "description": "Any short string."
+          }
+        },
+        "required": ["note"]
+      },
+      "result": "ocr_selftest ok"
+    },
     "timeout": 120
   }
-}
+}

internal/config/testconnection/testconnection.go

@@ -19,6 +19,20 @@ type TestTask struct {
 type LlmConversation struct {
 	Timeout  int           `json:"timeout"`
 	Messages []ChatMessage `json:"messages"`
+	// Tool is offered to the model so the test exercises a tool-call round
+	// trip. A single request cannot detect providers that reject the turn
+	// after a tool call, which is how #1357 passed the test but failed every
+	// review.
+	Tool *ToolSpec `json:"tool,omitempty"`
+}
+
+// ToolSpec is the throwaway tool offered during the connectivity test, together
+// with the canned result sent back as the second turn.
+type ToolSpec struct {
+	Name        string         `json:"name"`
+	Description string         `json:"description"`
+	Parameters  map[string]any `json:"parameters"`
+	Result      string         `json:"result"`
 }
 
 // ChatMessage represents a single message in a conversation.