mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-10-02 09:24:52 +08:00
feat(llm): exercise a tool-call round trip in ocr llm test (#1394)
A single request never covers the turn after a tool call, so a provider that rejects that turn passed the connectivity test while failing every review. Refs #1357 Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>
This commit is contained in:
MANAS RASTOGI
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user