diff --git a/cmd/opencodereview/llm_cmd.go b/cmd/opencodereview/llm_cmd.go index d4a96c126..7113b6fd9 100644 --- a/cmd/opencodereview/llm_cmd.go +++ b/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. diff --git a/cmd/opencodereview/llm_cmd_test.go b/cmd/opencodereview/llm_cmd_test.go index ddbd8ed3f..795fca633 100644 --- a/cmd/opencodereview/llm_cmd_test.go +++ b/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) diff --git a/internal/config/testconnection/task.json b/internal/config/testconnection/task.json index 12a169a4d..7a6c4af46 100644 --- a/internal/config/testconnection/task.json +++ b/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 } -} \ No newline at end of file +} diff --git a/internal/config/testconnection/testconnection.go b/internal/config/testconnection/testconnection.go index d6867d8a2..3d7282e12 100644 --- a/internal/config/testconnection/testconnection.go +++ b/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. diff --git a/internal/config/testconnection/testconnection_test.go b/internal/config/testconnection/testconnection_test.go index 0fcb59378..8e108b74f 100644 --- a/internal/config/testconnection/testconnection_test.go +++ b/internal/config/testconnection/testconnection_test.go @@ -93,3 +93,28 @@ func TestApplyLanguage_EmptyLang(t *testing.T) { t.Errorf("content = %q, want %q", conv.Messages[0].Content, expected) } } + +// TestLoadDefault_Tool guards the tool-call round trip added for #1357: the +// connectivity test only exercises the request shape that actually breaks if it +// offers a tool, so the embedded task must define one. +func TestLoadDefault_Tool(t *testing.T) { + conv, err := LoadDefault() + if err != nil { + t.Fatalf("LoadDefault: %v", err) + } + if conv.Tool == nil { + t.Fatal("expected the test task to define a tool") + } + if conv.Tool.Name == "" { + t.Error("tool name must not be empty") + } + if conv.Tool.Result == "" { + t.Error("tool must define the canned result sent back on the second turn") + } + if conv.Tool.Parameters == nil { + t.Fatal("tool must define a JSON Schema parameters object") + } + if got := conv.Tool.Parameters["type"]; got != "object" { + t.Errorf("tool parameters type = %v, want object", got) + } +}