diff --git a/cmd/opencodereview/emit_run_result_test.go b/cmd/opencodereview/emit_run_result_test.go index ce4fb1e4f..728b5431e 100644 --- a/cmd/opencodereview/emit_run_result_test.go +++ b/cmd/opencodereview/emit_run_result_test.go @@ -319,6 +319,44 @@ func TestEmitRunResult_JSONWithComments(t *testing.T) { } } +func TestEmitRunResult_DoesNotResolveAmbiguousCommentToFirstMatch(t *testing.T) { + ag := &mockResultProvider{ + filesReviewed: 1, + diffs: []model.Diff{{ + NewPath: "main.go", + Diff: `@@ -1,6 +1,8 @@ + func first() { ++ target() + } + func second() { ++ target() + } +`, + }}, + } + comments := []model.LlmComment{{ + Path: "main.go", + Content: "ambiguous repeated code", + ExistingCode: "target()", + }} + + got := captureStdout(t, func() { + if err := emitRunResult(context.Background(), ag, comments, time.Now(), "json", "developer", nil, nil, nil); err != nil { + t.Fatalf("emitRunResult: %v", err) + } + }) + var out jsonOutput + if err := json.Unmarshal([]byte(got), &out); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(out.Comments) != 1 { + t.Fatalf("comments = %d, want 1", len(out.Comments)) + } + if out.Comments[0].StartLine != 0 || out.Comments[0].EndLine != 0 { + t.Fatalf("comment lines = %d-%d, want unresolved 0-0", out.Comments[0].StartLine, out.Comments[0].EndLine) + } +} + func TestEmitRunResult_JSONWithResumeInfo(t *testing.T) { ag := &mockResultProvider{ filesReviewed: 2, diff --git a/internal/config/template/prompts/candidate_re_location_task_system.md b/internal/config/template/prompts/candidate_re_location_task_system.md new file mode 100644 index 000000000..58d3f0184 --- /dev/null +++ b/internal/config/template/prompts/candidate_re_location_task_system.md @@ -0,0 +1 @@ +You are a code location assistant. Given a review comment and candidate code locations, choose the single candidate that the comment targets. /no_think diff --git a/internal/config/template/prompts/candidate_re_location_task_user.md b/internal/config/template/prompts/candidate_re_location_task_user.md new file mode 100644 index 000000000..53d68fee1 --- /dev/null +++ b/internal/config/template/prompts/candidate_re_location_task_user.md @@ -0,0 +1,41 @@ +A review comment's existing_code matched multiple locations. Choose the one location the comment actually targets. + +Rules: +1. Use the review comment, suggestion, optional reasoning, and candidate context to decide. +2. Return ONLY one JSON object, no Markdown and no explanation. +3. If no candidate is clearly correct, return {"candidate_id":null}. +4. Candidates are provided as a JSON array. Use only a candidate_id value from that array. + +Output schema: +{"candidate_id":1} + +Examples: + +Input candidates: +[{"candidate_id":"1"},{"candidate_id":"2"}] +Correct output when candidate 2 is the target: +{"candidate_id":2} + +Input candidates: +[{"candidate_id":"1"},{"candidate_id":"2"}] +Correct output when neither candidate is clearly the target: +{"candidate_id":null} + +Review comment: +{suggestion_content} + +Original existing_code: +``` +{existing_code} +``` + +Suggestion: +``` +{suggestion_code} +``` + +Reviewer reasoning: +{thinking} + +Candidates: +{candidates} diff --git a/internal/config/template/task_template.json b/internal/config/template/task_template.json index 8a9c1dd38..99230fbac 100644 --- a/internal/config/template/task_template.json +++ b/internal/config/template/task_template.json @@ -29,6 +29,12 @@ { "role": "user", "prompt_file": "re_location_task_user.md" } ] }, + "CANDIDATE_RE_LOCATION_TASK": { + "messages": [ + { "role": "system", "prompt_file": "candidate_re_location_task_system.md" }, + { "role": "user", "prompt_file": "candidate_re_location_task_user.md" } + ] + }, "MAX_TOOL_REQUEST_TIMES": 30, "PLAN_MODE_LINE_THRESHOLD": 50, "MAX_TOKENS": 58888 diff --git a/internal/config/template/template.go b/internal/config/template/template.go index 2ca8f3086..b0bb9c9d7 100644 --- a/internal/config/template/template.go +++ b/internal/config/template/template.go @@ -20,11 +20,12 @@ type Template struct { MaxTokens int `json:"MAX_TOKENS"` // MaxCompletionTokens is a runtime-only output cap. When zero, callers // retain the template's historical MaxTokens behavior. - MaxCompletionTokens int `json:"-"` - MaxToolRequestTimes int `json:"MAX_TOOL_REQUEST_TIMES"` - PlanModeLineThreshold int `json:"PLAN_MODE_LINE_THRESHOLD"` - ReLocationTask *LlmConversation `json:"RE_LOCATION_TASK,omitempty"` - ReviewFilterTask *LlmConversation `json:"REVIEW_FILTER_TASK,omitempty"` + MaxCompletionTokens int `json:"-"` + MaxToolRequestTimes int `json:"MAX_TOOL_REQUEST_TIMES"` + PlanModeLineThreshold int `json:"PLAN_MODE_LINE_THRESHOLD"` + ReLocationTask *LlmConversation `json:"RE_LOCATION_TASK,omitempty"` + CandidateReLocationTask *LlmConversation `json:"CANDIDATE_RE_LOCATION_TASK,omitempty"` + ReviewFilterTask *LlmConversation `json:"REVIEW_FILTER_TASK,omitempty"` } // ScanTemplate holds the full-file scan task template configuration loaded @@ -82,14 +83,15 @@ type manifestConversation struct { } type templateManifest struct { - MainTask manifestConversation `json:"MAIN_TASK"` - PlanTask *manifestConversation `json:"PLAN_TASK,omitempty"` - MemoryCompressionTask manifestConversation `json:"MEMORY_COMPRESSION_TASK"` - MaxTokens int `json:"MAX_TOKENS"` - MaxToolRequestTimes int `json:"MAX_TOOL_REQUEST_TIMES"` - PlanModeLineThreshold int `json:"PLAN_MODE_LINE_THRESHOLD"` - ReLocationTask *manifestConversation `json:"RE_LOCATION_TASK,omitempty"` - ReviewFilterTask *manifestConversation `json:"REVIEW_FILTER_TASK,omitempty"` + MainTask manifestConversation `json:"MAIN_TASK"` + PlanTask *manifestConversation `json:"PLAN_TASK,omitempty"` + MemoryCompressionTask manifestConversation `json:"MEMORY_COMPRESSION_TASK"` + MaxTokens int `json:"MAX_TOKENS"` + MaxToolRequestTimes int `json:"MAX_TOOL_REQUEST_TIMES"` + PlanModeLineThreshold int `json:"PLAN_MODE_LINE_THRESHOLD"` + ReLocationTask *manifestConversation `json:"RE_LOCATION_TASK,omitempty"` + CandidateReLocationTask *manifestConversation `json:"CANDIDATE_RE_LOCATION_TASK,omitempty"` + ReviewFilterTask *manifestConversation `json:"REVIEW_FILTER_TASK,omitempty"` } func resolveConversation(m manifestConversation) (LlmConversation, error) { @@ -147,6 +149,9 @@ func LoadDefault() (*Template, error) { if tpl.ReLocationTask, err = resolveOptionalConversation(m.ReLocationTask, "RE_LOCATION_TASK"); err != nil { return nil, err } + if tpl.CandidateReLocationTask, err = resolveOptionalConversation(m.CandidateReLocationTask, "CANDIDATE_RE_LOCATION_TASK"); err != nil { + return nil, err + } if tpl.ReviewFilterTask, err = resolveOptionalConversation(m.ReviewFilterTask, "REVIEW_FILTER_TASK"); err != nil { return nil, err } diff --git a/internal/config/template/template_test.go b/internal/config/template/template_test.go index ae3f07956..4bce221fc 100644 --- a/internal/config/template/template_test.go +++ b/internal/config/template/template_test.go @@ -93,6 +93,9 @@ func TestLoadDefault_FieldsPopulated(t *testing.T) { if tpl.ReLocationTask == nil { t.Fatal("ReLocationTask is nil, expected non-nil") } + if tpl.CandidateReLocationTask == nil { + t.Fatal("CandidateReLocationTask is nil, expected non-nil") + } if tpl.ReviewFilterTask == nil { t.Fatal("ReviewFilterTask is nil, expected non-nil") } @@ -124,6 +127,11 @@ func TestLoadDefault_PlaceholdersPresent(t *testing.T) { {"MemoryCompression user has context", tpl.MemoryCompressionTask.Messages[1].Content, "{{context}}"}, {"ReviewFilter user has comments", tpl.ReviewFilterTask.Messages[1].Content, "{{comments}}"}, {"ReLocation user has diff (single brace)", tpl.ReLocationTask.Messages[1].Content, "{diff}"}, + {"CandidateReLocation user has suggestion content", tpl.CandidateReLocationTask.Messages[1].Content, "{suggestion_content}"}, + {"CandidateReLocation user has candidates", tpl.CandidateReLocationTask.Messages[1].Content, "{candidates}"}, + {"CandidateReLocation user has existing_code", tpl.CandidateReLocationTask.Messages[1].Content, "{existing_code}"}, + {"CandidateReLocation user has suggestion_code", tpl.CandidateReLocationTask.Messages[1].Content, "{suggestion_code}"}, + {"CandidateReLocation user has thinking", tpl.CandidateReLocationTask.Messages[1].Content, "{thinking}"}, } for _, tt := range tests { diff --git a/internal/diff/relocation.go b/internal/diff/relocation.go index 9c3e8994b..2f4f8999d 100644 --- a/internal/diff/relocation.go +++ b/internal/diff/relocation.go @@ -5,6 +5,7 @@ package diff import ( "context" + "encoding/json" "fmt" "strings" "time" @@ -16,35 +17,112 @@ import ( "github.com/alibaba/open-code-review/internal/telemetry" ) -// BuildReLocationMessages renders the re-location prompt for cm against d. -// Returns nil when the task template is absent or empty, which the caller +type promptReplacement struct { + token string + value string +} + +const candidateReLocationRetryPrompt = `The previous answer was not valid for candidate selection. Return exactly one JSON object and nothing else. + +Use only a candidate_id from the candidate list. Return {"candidate_id":null} only when no candidate is clearly correct. Do not use Markdown, labels, or prose.` + +// BuildReLocationMessages renders the snippet-based re-location prompt. +// It returns nil when the task template is absent or empty, which the caller // treats as "no re-location attempt": no session record, no request. // -// This is split out of ReLocateComment so the caller can create the -// ReLocationTask session record — and therefore know its RequestNo — before any -// HTTP call happens. It is pure prompt construction: no client, no session, no -// request identity. Keeping it that way is what stops observability concerns -// from sinking into package diff. +// Prompt rendering stays separate from ReLocateComment so the caller can create +// the ReLocationTask session record, including RequestNo, before the HTTP call. +// Keeping this function pure also keeps session and request identity concerns +// out of package diff. func BuildReLocationMessages(cm *model.LlmComment, d *model.Diff, task *template.LlmConversation) []llm.Message { + return renderPromptMessages(task, []promptReplacement{ + {token: "{diff}", value: d.Diff}, + {token: "{existing_code}", value: cm.ExistingCode}, + {token: "{suggestion_content}", value: cm.Content}, + }) +} + +// BuildCandidateReLocationMessages renders the candidate-selection prompt. +func BuildCandidateReLocationMessages(cm *model.LlmComment, candidates []CommentLocationCandidate, task *template.LlmConversation) []llm.Message { + if len(candidates) == 0 { + return nil + } + + return renderPromptMessages(task, []promptReplacement{ + {token: "{suggestion_content}", value: cm.Content}, + {token: "{existing_code}", value: cm.ExistingCode}, + {token: "{suggestion_code}", value: strings.TrimSpace(cm.SuggestionCode)}, + {token: "{thinking}", value: strings.TrimSpace(cm.Thinking)}, + {token: "{candidates}", value: renderCandidateList(candidates)}, + }) +} + +// BuildCandidateReLocationRetryMessages appends a local repair turn after the +// model answered in the wrong format. The retry conversation is scoped to the +// re-location task only and is never appended to the main review loop. +func BuildCandidateReLocationRetryMessages(messages []llm.Message, previous string) []llm.Message { + out := append([]llm.Message(nil), messages...) + if previous = strings.TrimSpace(previous); previous != "" { + out = append(out, llm.NewTextMessage("assistant", previous)) + } + out = append(out, llm.NewTextMessage("user", candidateReLocationRetryPrompt)) + return out +} + +func renderPromptMessages(task *template.LlmConversation, replacements []promptReplacement) []llm.Message { if task == nil || len(task.Messages) == 0 { return nil } messages := make([]llm.Message, 0, len(task.Messages)) for _, m := range task.Messages { - content := m.Content - content = strings.ReplaceAll(content, "{diff}", d.Diff) - content = strings.ReplaceAll(content, "{existing_code}", cm.ExistingCode) - content = strings.ReplaceAll(content, "{suggestion_content}", cm.Content) - messages = append(messages, llm.NewTextMessage(m.Role, content)) + messages = append(messages, llm.NewTextMessage(m.Role, replacePromptTokens(m.Content, replacements))) } return messages } -// ReLocateComment calls the LLM to regenerate a precise existing_code snippet -// when text-based matching fails, then retries ResolveComment with the new -// snippet. messages comes from BuildReLocationMessages; the caller has already -// recorded it in the session, so only (success, response) are returned here. +func replacePromptTokens(content string, replacements []promptReplacement) string { + args := make([]string, 0, len(replacements)*2) + for _, r := range replacements { + args = append(args, r.token, r.value) + } + return strings.NewReplacer(args...).Replace(content) +} + +func renderCandidateList(candidates []CommentLocationCandidate) string { + type candidatePromptItem struct { + CandidateID string `json:"candidate_id"` + Path string `json:"path"` + StartLine int `json:"start_line"` + EndLine int `json:"end_line"` + MatchedCode string `json:"matched_code"` + Context string `json:"context,omitempty"` + } + + items := make([]candidatePromptItem, 0, len(candidates)) + for _, c := range candidates { + item := candidatePromptItem{ + CandidateID: c.ID, + Path: c.Path, + StartLine: c.StartLine, + EndLine: c.EndLine, + MatchedCode: c.Snippet, + } + if strings.TrimSpace(c.Context) != "" && c.Context != c.Snippet { + item.Context = c.Context + } + items = append(items, item) + } + data, err := json.MarshalIndent(items, "", " ") + if err != nil { + return "[]" + } + return string(data) +} + +// ReLocateComment asks the LLM to regenerate a precise existing_code snippet +// when text matching fails, then retries ResolveComment with the new snippet. +// The caller owns session recording, so this returns only success and response. // Response is nil when the request failed. func ReLocateComment( ctx context.Context, @@ -94,6 +172,144 @@ func ReLocateComment( return false, resp } +// ReLocateCommentCandidate asks the LLM to choose a precomputed candidate. +func ReLocateCommentCandidate( + ctx context.Context, + cm *model.LlmComment, + candidates []CommentLocationCandidate, + client llm.LLMClient, + messages []llm.Message, + modelName string, + maxTokens int, +) (bool, *llm.ChatResponse) { + if len(messages) == 0 || len(candidates) == 0 { + return false, nil + } + + startTime := time.Now() + _, llmSpan := telemetry.StartLLMSpan(ctx, modelName) + resp, err := client.CompletionsWithCtx(ctx, llm.ChatRequest{ + Model: modelName, + Messages: messages, + MaxTokens: maxTokens, + }) + duration := time.Since(startTime) + if err != nil { + telemetry.RecordLLMResult(llmSpan, duration, 0, err) + llmSpan.End() + fmt.Fprintf(stdout.Writer(), "[ocr] Re-location candidate selection failed for %s: %v\n", cm.Path, err) + return false, nil + } + var totalTokens int64 + if resp.Usage != nil { + totalTokens = resp.Usage.TotalTokens + } + telemetry.RecordLLMResult(llmSpan, duration, totalTokens, nil) + llmSpan.End() + + id, ok := ParseCandidateID(resp.Content()) + if !ok || id == "" { + return false, resp + } + for _, c := range candidates { + if c.ID == id { + ApplyCandidate(cm, c) + return true, resp + } + } + return false, resp +} + +// ParseCandidateID reports whether content follows the strict candidate output +// contract: a JSON object with candidate_id. +func ParseCandidateID(content string) (string, bool) { + content = strings.TrimSpace(content) + if content == "" { + return "", false + } + if id, ok := parseCandidateIDJSON(content); ok { + return id, true + } + if block := extractWholeCodeBlock(content); block != "" { + return parseCandidateIDJSON(block) + } + return "", false +} + +func parseCandidateIDJSON(content string) (string, bool) { + var payload map[string]json.RawMessage + if err := json.Unmarshal([]byte(content), &payload); err == nil && payload != nil { + raw, ok := payload["candidate_id"] + if !ok { + return "", false + } + var value any + if err := json.Unmarshal(raw, &value); err != nil { + return "", false + } + return normalizeCandidateID(value) + } + return "", false +} + +func extractWholeCodeBlock(text string) string { + text = strings.TrimSpace(text) + if !strings.HasPrefix(text, "```") { + return "" + } + afterOpen := 3 + if nl := strings.IndexByte(text[afterOpen:], '\n'); nl >= 0 { + afterOpen += nl + 1 + } else { + return "" + } + end := strings.Index(text[afterOpen:], "```") + if end < 0 { + return "" + } + afterClose := afterOpen + end + 3 + if strings.TrimSpace(text[afterClose:]) != "" { + return "" + } + return strings.TrimSpace(text[afterOpen : afterOpen+end]) +} + +func normalizeCandidateID(value any) (string, bool) { + switch v := value.(type) { + case nil: + return "", true + case string: + v = strings.TrimSpace(v) + if v == "" { + return "", false + } + if !isPositiveInteger(v) { + return "", false + } + return v, true + case float64: + n := int64(v) + if v < 1 || v != float64(n) { + return "", false + } + return fmt.Sprintf("%d", n), true + default: + return "", false + } +} + +func isPositiveInteger(s string) bool { + for i, r := range s { + if r < '0' || r > '9' { + return false + } + if i == 0 && r == '0' { + return false + } + } + return s != "" +} + // extractCodeBlock extracts the content of the first fenced code block from text. // Returns empty string if no code block is found. func extractCodeBlock(text string) string { diff --git a/internal/diff/relocation_test.go b/internal/diff/relocation_test.go index 3df0cf52f..ff6edd324 100644 --- a/internal/diff/relocation_test.go +++ b/internal/diff/relocation_test.go @@ -5,7 +5,9 @@ package diff import ( "context" + "encoding/json" "errors" + "strings" "testing" "github.com/alibaba/open-code-review/internal/config/template" @@ -36,7 +38,16 @@ func makeTask() *template.LlmConversation { return &template.LlmConversation{ Messages: []template.ChatMessage{ {Role: "system", Content: "you are a helper"}, - {Role: "user", Content: "diff:\n{diff}\n\ncomment:\n{suggestion_content}"}, + {Role: "user", Content: "diff:\n{diff}\ncode:\n{existing_code}\nsuggestion:\n{suggestion_content}"}, + }, + } +} + +func makeCandidateTask() *template.LlmConversation { + return &template.LlmConversation{ + Messages: []template.ChatMessage{ + {Role: "system", Content: "select candidate"}, + {Role: "user", Content: "comment:\n{suggestion_content}\ncode:\n{existing_code}\nsuggestion:\n{suggestion_code}\nthinking:\n{thinking}\ncandidates:\n{candidates}"}, }, } } @@ -129,6 +140,198 @@ func TestReLocateComment_LLMReturnsValidCode(t *testing.T) { } } +func TestReLocateCommentCandidate_SelectsPrecomputedLocation(t *testing.T) { + cm := model.LlmComment{ + Path: "main.go", + Content: "The second branch should not fail after success.", + ExistingCode: "status = failed", + SuggestionCode: "status = succeeded", + } + candidates := []CommentLocationCandidate{ + {ID: "1", Path: "main.go", StartLine: 10, EndLine: 10, Snippet: "status = failed", Context: "if err != nil {\nstatus = failed\n}"}, + {ID: "2", Path: "main.go", StartLine: 42, EndLine: 44, Snippet: "status = failed", Context: "if remoteStatus == \"\" {\nstatus = failed\n}"}, + } + client := &mockLLMClient{response: newMockResponse(`{"candidate_id":2}`)} + msgs := BuildCandidateReLocationMessages(&cm, candidates, makeCandidateTask()) + if len(msgs) != 2 { + t.Fatalf("messages = %d, want 2", len(msgs)) + } + if !strings.Contains(msgs[1].ExtractText(), "remoteStatus") { + t.Fatalf("candidate prompt did not include context: %s", msgs[1].ExtractText()) + } + + ok, resp := ReLocateCommentCandidate(context.Background(), &cm, candidates, client, msgs, "test-model", 1000) + if !ok { + t.Fatal("expected candidate re-location to succeed") + } + if resp == nil { + t.Fatal("expected non-nil response") + } + if cm.StartLine != 42 || cm.EndLine != 44 { + t.Fatalf("lines = %d-%d, want 42-44", cm.StartLine, cm.EndLine) + } +} + +func TestBuildCandidateReLocationMessages_RendersCandidatesAsJSON(t *testing.T) { + cm := model.LlmComment{ + Path: "main.go", + Content: "The second branch is wrong.", + ExistingCode: "target()", + } + candidates := []CommentLocationCandidate{ + { + ID: "1", + Path: "main.go", + StartLine: 10, + EndLine: 12, + Snippet: "fmt.Println(```)", + Context: "payload := `{\"candidate_id\":1}`\nfmt.Println(```)", + }, + } + + msgs := BuildCandidateReLocationMessages(&cm, candidates, makeCandidateTask()) + if len(msgs) != 2 { + t.Fatalf("messages = %d, want 2", len(msgs)) + } + text := msgs[1].ExtractText() + marker := "candidates:\n" + start := strings.Index(text, marker) + if start < 0 { + t.Fatalf("candidate prompt missing candidate section: %s", text) + } + candidateJSON := text[start+len(marker):] + if strings.Contains(candidateJSON, "matched code:\n```") || strings.Contains(candidateJSON, "candidate context:\n```") { + t.Fatalf("candidate list should not use Markdown fences as structure: %s", candidateJSON) + } + var rendered []struct { + CandidateID string `json:"candidate_id"` + MatchedCode string `json:"matched_code"` + Context string `json:"context"` + } + if err := json.Unmarshal([]byte(candidateJSON), &rendered); err != nil { + t.Fatalf("candidate list is not valid JSON: %v\n%s", err, candidateJSON) + } + if len(rendered) != 1 || rendered[0].CandidateID != "1" { + t.Fatalf("rendered candidates = %+v, want candidate 1", rendered) + } + if rendered[0].MatchedCode != "fmt.Println(```)" || !strings.Contains(rendered[0].Context, `{"candidate_id":1}`) { + t.Fatalf("rendered candidate lost code content: %+v", rendered[0]) + } +} + +func TestBuildCandidateReLocationMessages_DoesNotExpandInsertedPlaceholders(t *testing.T) { + cm := model.LlmComment{ + Path: "main.go", + Content: "Do not replace this literal token: {candidates}", + ExistingCode: "target()", + SuggestionCode: "Do not replace this literal token either: {thinking}", + Thinking: "Keep {existing_code} as plain text.", + } + candidates := []CommentLocationCandidate{{ + ID: "1", + Path: "main.go", + StartLine: 10, + EndLine: 10, + Snippet: "target()", + }} + + msgs := BuildCandidateReLocationMessages(&cm, candidates, makeCandidateTask()) + if len(msgs) != 2 { + t.Fatalf("messages = %d, want 2", len(msgs)) + } + text := msgs[1].ExtractText() + if !strings.Contains(text, "Do not replace this literal token: {candidates}") { + t.Fatalf("suggestion content placeholder literal was re-expanded: %s", text) + } + if !strings.Contains(text, "Do not replace this literal token either: {thinking}") { + t.Fatalf("suggestion code placeholder literal was re-expanded: %s", text) + } + if !strings.Contains(text, "Keep {existing_code} as plain text.") { + t.Fatalf("thinking placeholder literal was re-expanded: %s", text) + } +} + +func TestReLocateCommentCandidate_NullCandidateDeclines(t *testing.T) { + cm := model.LlmComment{Path: "main.go", Content: "issue", ExistingCode: "x"} + candidates := []CommentLocationCandidate{{ID: "1", Path: "main.go", StartLine: 1, EndLine: 1, Snippet: "x"}} + client := &mockLLMClient{response: newMockResponse(`{"candidate_id":null}`)} + + ok, resp := ReLocateCommentCandidate(context.Background(), &cm, candidates, client, BuildCandidateReLocationMessages(&cm, candidates, makeCandidateTask()), "test-model", 1000) + if ok { + t.Fatal("expected null candidate to decline") + } + if resp == nil { + t.Fatal("expected response to be recorded") + } + if cm.StartLine != 0 || cm.EndLine != 0 { + t.Fatalf("lines = %d-%d, want 0-0", cm.StartLine, cm.EndLine) + } +} + +func TestReLocateCommentCandidate_StrictParserRejectsProse(t *testing.T) { + cm := model.LlmComment{Path: "main.go", Content: "issue", ExistingCode: "x"} + candidates := []CommentLocationCandidate{{ID: "2", Path: "main.go", StartLine: 2, EndLine: 2, Snippet: "x"}} + client := &mockLLMClient{response: newMockResponse("The candidate is 2.")} + + ok, resp := ReLocateCommentCandidate(context.Background(), &cm, candidates, client, BuildCandidateReLocationMessages(&cm, candidates, makeCandidateTask()), "test-model", 1000) + if ok { + t.Fatal("expected strict parser to reject prose") + } + if resp == nil { + t.Fatal("expected response to be returned") + } + if cm.StartLine != 0 || cm.EndLine != 0 { + t.Fatalf("lines = %d-%d, want 0-0", cm.StartLine, cm.EndLine) + } +} + +func TestParseCandidateID_StrictJSONObjectOnly(t *testing.T) { + tests := []struct { + name string + content string + wantID string + wantParsed bool + }{ + {name: "number id", content: `{"candidate_id":2}`, wantID: "2", wantParsed: true}, + {name: "string id", content: `{"candidate_id":"2"}`, wantID: "2", wantParsed: true}, + {name: "null id", content: `{"candidate_id":null}`, wantParsed: true}, + {name: "string null", content: `{"candidate_id":"null"}`, wantParsed: false}, + {name: "prose", content: "Candidate 2 is correct.", wantParsed: false}, + {name: "bare number", content: "2", wantParsed: false}, + {name: "fenced json", content: "```json\n{\"candidate_id\":2}\n```", wantID: "2", wantParsed: true}, + {name: "fenced json with prose", content: "Here is the answer:\n```json\n{\"candidate_id\":2}\n```", wantParsed: false}, + {name: "missing field", content: `{}`, wantParsed: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotID, gotParsed := ParseCandidateID(tt.content) + if gotID != tt.wantID || gotParsed != tt.wantParsed { + t.Fatalf("ParseCandidateID() = %q/%v, want %q/%v", gotID, gotParsed, tt.wantID, tt.wantParsed) + } + }) + } +} + +func TestBuildCandidateReLocationRetryMessages(t *testing.T) { + base := []llm.Message{ + llm.NewTextMessage("system", "select"), + llm.NewTextMessage("user", "candidates"), + } + got := BuildCandidateReLocationRetryMessages(base, "candidate 2") + if len(got) != 4 { + t.Fatalf("messages = %d, want 4", len(got)) + } + if got[2].Role != "assistant" || got[2].ExtractText() != "candidate 2" { + t.Fatalf("assistant retry context = %q/%q", got[2].Role, got[2].ExtractText()) + } + if got[3].Role != "user" || + !strings.Contains(got[3].ExtractText(), "Return exactly one JSON object") || + !strings.Contains(got[3].ExtractText(), `{"candidate_id":null}`) { + t.Fatalf("retry instruction = %q/%q", got[3].Role, got[3].ExtractText()) + } +} + func TestReLocateComment_LLMReturnsInvalidContent(t *testing.T) { cm := model.LlmComment{ Path: "main.go", diff --git a/internal/diff/resolver.go b/internal/diff/resolver.go index 8443dfd70..b9f16688f 100644 --- a/internal/diff/resolver.go +++ b/internal/diff/resolver.go @@ -4,6 +4,7 @@ package diff import ( + "strconv" "strings" "github.com/alibaba/open-code-review/internal/model" @@ -45,13 +46,10 @@ func ResolveLineNumbers(comments []model.LlmComment, diffs []model.Diff) []model continue } - // Primary: try matching from deleted/context lines in diff hunks - if resolveFromHunk(d, cm) { - continue + candidates := ResolveCommentCandidates(cm, d) + if len(candidates) == 1 { + ApplyCandidate(cm, candidates[0]) } - - // Fallback: scan the new file content for consecutive matches - resolveFromFileContent(d, cm) } return result @@ -72,6 +70,80 @@ func ResolveComment(cm *model.LlmComment, d *model.Diff) bool { return resolveFromFileContent(d, cm) } +// CommentLocationCandidate is one place where ExistingCode matched. +type CommentLocationCandidate struct { + ID string + Path string + StartLine int + EndLine int + Snippet string + Context string +} + +// ResolveCommentCandidates returns locations that match cm.ExistingCode in d, +// using ResolveComment's existing search order while keeping ambiguity visible: +// new-side hunks, old-side hunks, then full new-file content. +func ResolveCommentCandidates(cm *model.LlmComment, d *model.Diff) []CommentLocationCandidate { + return assignCandidateIDs(commentCandidates(cm, d)) +} + +func commentCandidates(cm *model.LlmComment, d *model.Diff) []CommentLocationCandidate { + if cm == nil || d == nil || cm.ExistingCode == "" { + return nil + } + if cm.StartLine > 0 || cm.EndLine > 0 { + return []CommentLocationCandidate{{ + Path: candidatePath(d), + StartLine: cm.StartLine, + EndLine: cm.EndLine, + Snippet: cm.ExistingCode, + }} + } + + targetLines := splitAndNormalize(cm.ExistingCode) + if len(targetLines) == 0 { + return nil + } + + hunks := ParseHunks(d.Diff) + if candidates := collectHunkCandidates(hunks, targetLines, d, true); len(candidates) > 0 { + return candidates + } + + if candidates := collectHunkCandidates(hunks, targetLines, d, false); len(candidates) > 0 { + return candidates + } + + return fileContentCandidates(d, targetLines) +} + +// ApplyCandidate mutates cm to point at c. +func ApplyCandidate(cm *model.LlmComment, c CommentLocationCandidate) { + if cm == nil { + return + } + if c.Path != "" { + cm.Path = c.Path + } + cm.StartLine = c.StartLine + cm.EndLine = c.EndLine +} + +// RelocationCandidates returns candidate anchors across the reviewed diff set. +func RelocationCandidates(cm *model.LlmComment, diffs []model.Diff) []CommentLocationCandidate { + if cm == nil || cm.ExistingCode == "" || len(diffs) == 0 { + return nil + } + var candidates []CommentLocationCandidate + for i := range diffs { + d := &diffs[i] + probe := *cm + probe.StartLine, probe.EndLine = 0, 0 + candidates = append(candidates, commentCandidates(&probe, d)...) + } + return assignCandidateIDs(candidates) +} + // RelocateAcrossFiles handles the comment whose ExistingCode belongs to a // different file than the one it was filed against. // @@ -138,6 +210,85 @@ func RelocateAcrossFiles(cm *model.LlmComment, diffs []model.Diff) (string, bool return hits[0].path, true } +func candidatePath(d *model.Diff) string { + if d == nil { + return "" + } + if d.NewPath != "" && d.NewPath != "/dev/null" { + return d.NewPath + } + if d.OldPath != "" && d.OldPath != "/dev/null" { + return d.OldPath + } + return "" +} + +func collectHunkCandidates(hunks []Hunk, targetLines []string, d *model.Diff, newSide bool) []CommentLocationCandidate { + var candidates []CommentLocationCandidate + for i := range hunks { + sideLines := extractSideLines(&hunks[i], newSide) + candidates = append(candidates, candidatesFromIndexedLines(sideLines, targetLines, d)...) + } + return candidates +} + +func candidatesFromIndexedLines(sideLines []indexedLine, targetLines []string, d *model.Diff) []CommentLocationCandidate { + if len(targetLines) == 0 || len(sideLines) < len(targetLines) { + return nil + } + + var candidates []CommentLocationCandidate + for i := 0; i <= len(sideLines)-len(targetLines); i++ { + if !indexedLinesMatch(sideLines[i:], targetLines) { + continue + } + contextStart := max(0, i-3) + contextEnd := min(len(sideLines), i+len(targetLines)+3) + candidates = append(candidates, CommentLocationCandidate{ + Path: candidatePath(d), + StartLine: sideLines[i].lineNum, + EndLine: sideLines[i+len(targetLines)-1].lineNum, + Snippet: snippetFromIndexedLines(sideLines[i : i+len(targetLines)]), + Context: snippetFromIndexedLines(sideLines[contextStart:contextEnd]), + }) + } + return candidates +} + +func indexedLinesMatch(sideLines []indexedLine, targetLines []string) bool { + if len(sideLines) < len(targetLines) { + return false + } + for i, target := range targetLines { + if sideLines[i].content != target { + return false + } + } + return true +} + +func snippetFromIndexedLines(lines []indexedLine) string { + if len(lines) == 0 { + return "" + } + parts := make([]string, 0, len(lines)) + for _, l := range lines { + parts = append(parts, l.content) + } + return strings.Join(parts, "\n") +} + +func assignCandidateIDs(candidates []CommentLocationCandidate) []CommentLocationCandidate { + for i := range candidates { + candidates[i].ID = candidateID(i) + } + return candidates +} + +func candidateID(i int) string { + return strconv.Itoa(i + 1) +} + // indexedLine pairs a normalized line with its absolute file line number. type indexedLine struct { lineNum int @@ -282,6 +433,24 @@ func resolveFromFileContent(d *model.Diff, cm *model.LlmComment) bool { return false } +func fileContentCandidates(d *model.Diff, targetLines []string) []CommentLocationCandidate { + if d == nil || d.NewFileContent == "" || len(targetLines) == 0 { + return nil + } + + fileLines := strings.Split(d.NewFileContent, "\n") + sideLines := make([]indexedLine, 0, len(fileLines)) + for i, line := range fileLines { + n := normalizeLine(strings.TrimRight(line, "\r")) + if n == "" { + continue + } + sideLines = append(sideLines, indexedLine{lineNum: i + 1, content: n}) + } + + return candidatesFromIndexedLines(sideLines, targetLines, d) +} + // splitAndNormalize splits code text into lines and normalizes each one. func splitAndNormalize(code string) []string { raw := strings.Split(code, "\n") diff --git a/internal/diff/resolver_test.go b/internal/diff/resolver_test.go index ac5ba85cf..8ade04f0f 100644 --- a/internal/diff/resolver_test.go +++ b/internal/diff/resolver_test.go @@ -4,6 +4,7 @@ package diff import ( + "strings" "testing" "github.com/alibaba/open-code-review/internal/model" @@ -193,7 +194,7 @@ func TestResolveLineNumbers_FallbackToFileContent_CRLF(t *testing.T) { } } -func TestResolveLineNumbers_FallbackToFileContent_FirstMatchWins(t *testing.T) { +func TestResolveLineNumbers_FallbackToFileContent_DuplicateMatchStaysUnresolved(t *testing.T) { diffs := []model.Diff{{ NewPath: "main.go", NewFileContent: "x\ny\nx\ny\n", @@ -206,8 +207,66 @@ func TestResolveLineNumbers_FallbackToFileContent_FirstMatchWins(t *testing.T) { result := ResolveLineNumbers(comments, diffs) cm := result[0] - if cm.StartLine != 1 || cm.EndLine != 2 { - t.Errorf("first match wins: expected 1..2, got %d..%d", cm.StartLine, cm.EndLine) + if cm.StartLine != 0 || cm.EndLine != 0 { + t.Errorf("duplicate match should stay unresolved, got %d..%d", cm.StartLine, cm.EndLine) + } +} + +func TestResolveCommentCandidates_ReturnsAllHunkMatchesWithContext(t *testing.T) { + d := &model.Diff{ + NewPath: "main.go", + Diff: `@@ -10,6 +10,10 @@ + func first() { ++ beforeFirst() ++ target() ++ afterFirst() + } +@@ -40,6 +44,10 @@ + func second() { ++ beforeSecond() ++ target() ++ afterSecond() + } +`, + } + cm := &model.LlmComment{Path: "main.go", ExistingCode: "target()"} + + got := ResolveCommentCandidates(cm, d) + if len(got) != 2 { + t.Fatalf("got %d candidates, want 2", len(got)) + } + if got[0].ID != "1" || got[1].ID != "2" { + t.Fatalf("candidate ids = %q/%q, want 1/2", got[0].ID, got[1].ID) + } + if got[0].StartLine != 12 || got[1].StartLine != 46 { + t.Fatalf("candidate lines = %d/%d, want 12/46", got[0].StartLine, got[1].StartLine) + } + if !strings.Contains(got[1].Context, "beforeSecond()") || !strings.Contains(got[1].Context, "afterSecond()") { + t.Fatalf("second context = %q, want surrounding second function lines", got[1].Context) + } +} + +func TestResolveLineNumbers_DuplicateHunkMatchStaysUnresolved(t *testing.T) { + diffs := []model.Diff{{ + NewPath: "main.go", + Diff: `@@ -1,6 +1,8 @@ + func first() { ++ target() + } + func second() { ++ target() + } +`, + }} + comments := []model.LlmComment{{ + Path: "main.go", + Content: "ambiguous", + ExistingCode: "target()", + }} + + result := ResolveLineNumbers(comments, diffs) + if result[0].StartLine != 0 || result[0].EndLine != 0 { + t.Fatalf("duplicate hunk match was resolved to %d-%d; want 0-0", result[0].StartLine, result[0].EndLine) } } diff --git a/internal/llmloop/loop.go b/internal/llmloop/loop.go index e9de04164..173fec103 100644 --- a/internal/llmloop/loop.go +++ b/internal/llmloop/loop.go @@ -6,6 +6,7 @@ package llmloop import ( "context" "encoding/json" + "errors" "fmt" "sync" "sync/atomic" @@ -40,13 +41,11 @@ type Deps struct { // NewFileContent is the whole file and Diff is empty). DiffLookup func(path string) *model.Diff - // AllDiffs returns every diff this run reviews, for re-filing a comment - // whose ExistingCode belongs to a different file than the one it was filed - // against (diff.RelocateAcrossFiles). It is the reviewed set rather than - // every parsed diff on purpose: re-filing a comment onto a path the run - // excluded would point the reader at a file this review never covered. - // When nil, cross-file re-filing is skipped and only same-file resolution - // applies. + // AllDiffs returns every diff this run reviews, so duplicate anchors can be + // collected across files when the comment's current file has no match. It is + // the reviewed set rather than every parsed diff on purpose: re-filing a + // comment onto a path the run excluded would point the reader at a file this + // review never covered. When nil, only same-file resolution applies. AllDiffs func() []model.Diff // NewRequestMeta builds the retry-report identity for one logical LLM @@ -195,6 +194,57 @@ func (r *Runner) RecordUsage(u *llm.UsageInfo) { atomic.AddInt64(&r.totalCacheWriteTokens, u.CacheWriteTokens) } +func (r *Runner) runReLocationTask( + ctx context.Context, + filePath string, + messages []llm.Message, + call func(context.Context) (bool, *llm.ChatResponse), + errorMessage string, +) (bool, *llm.ChatResponse) { + if len(messages) == 0 { + return false, nil + } + + startTime := time.Now() + fs := r.deps.Session.GetOrCreateFileSession(filePath) + rec := fs.AppendTaskRecord(session.ReLocationTask, messages) + taskCtx := llm.ContextWithSessionKey(ctx, + llm.SessionTaskKey(r.deps.Session.SessionID, string(session.ReLocationTask), filePath)) + reqCtx := r.requestCtx(taskCtx, filePath, session.ReLocationTask, rec.RequestNo) + + ok, resp := call(reqCtx) + if resp != nil { + rec.SetResponse(resp, time.Since(startTime)) + r.RecordUsage(resp.Usage) + return ok, resp + } + rec.SetError(errors.New(errorMessage), time.Since(startTime)) + return ok, nil +} + +func needsCandidateReLocationRetry(content string, candidates []diff.CommentLocationCandidate) bool { + id, parsed := diff.ParseCandidateID(content) + if !parsed { + return true + } + if id == "" { + return false + } + for _, c := range candidates { + if c.ID == id { + return false + } + } + return true +} + +func candidateReLocationMessagesForRetry(messages []llm.Message, resp *llm.ChatResponse) []llm.Message { + if resp == nil { + return messages + } + return diff.BuildCandidateReLocationRetryMessages(messages, resp.Content()) +} + // CollectPendingComments awaits any async comment-processing workers and // returns the aggregated comments from the collector. Safe to call once // per session at the end. @@ -557,54 +607,60 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T if r.deps.DiffLookup != nil { d = r.deps.DiffLookup(cm.Path) } - // Resolution order: the comment's own file, then a cross-file - // search, then the LLM. The cross-file search precedes the LLM - // because it needs the Agent's original ExistingCode, which the - // LLM step overwrites; and it runs even when d is nil, since a - // comment filed against a path this run holds no diff for is - // exactly the case that search can still place. - located := d != nil && diff.ResolveComment(cm, d) - if !located && r.deps.AllDiffs != nil { + var candidates []diff.CommentLocationCandidate + if d != nil { + candidates = diff.ResolveCommentCandidates(cm, d) + } + // Keep the current file authoritative: a local match must not + // become ambiguous just because another reviewed file contains + // the same snippet. + if len(candidates) == 0 && r.deps.AllDiffs != nil { + candidates = diff.RelocationCandidates(cm, r.deps.AllDiffs()) + } + located := false + if len(candidates) == 1 { from := cm.Path - if to, ok := diff.RelocateAcrossFiles(cm, r.deps.AllDiffs()); ok { - located = true - r.RecordWarning("comment_refiled", to, fmt.Sprintf( - "comment filed against %s describes code in %s; re-filed", from, to)) + diff.ApplyCandidate(cm, candidates[0]) + located = true + if cm.Path != from { + r.RecordWarning("comment_refiled", cm.Path, fmt.Sprintf( + "comment filed against %s describes code in %s; re-filed", from, cm.Path)) } } - if d != nil { - if !located && r.deps.Template.ReLocationTask != nil { - // rlStart stays ahead of prompt construction, which is - // where it sat when ReLocateComment built the messages - // itself — moving it would silently change what - // TaskRecord.Duration measures. - rlStart := time.Now() - msgs := diff.BuildReLocationMessages(cm, d, r.deps.Template.ReLocationTask) - if len(msgs) > 0 { - fs := r.deps.Session.GetOrCreateFileSession(cm.Path) - rlRec := fs.AppendTaskRecord(session.ReLocationTask, msgs) - // FilePath is cm.Path so it cannot drift from the file - // session opened above — that join is what the report - // needs. It equals newPath whenever newPath is set, - // because the path arg is overridden with it further - // up, but reading it from the comment keeps the two - // aligned without depending on that. - rlCtx := llm.ContextWithSessionKey(rctx, - llm.SessionTaskKey(r.deps.Session.SessionID, string(session.ReLocationTask), cm.Path)) - reqCtx := r.requestCtx(rlCtx, cm.Path, session.ReLocationTask, rlRec.RequestNo) - _, resp := diff.ReLocateComment(reqCtx, cm, d, r.deps.LLMClient, msgs, r.deps.Model, r.deps.Template.CompletionTokenLimit()) - if resp != nil { - rlRec.SetResponse(resp, time.Since(rlStart)) - if resp.Usage != nil { - atomic.AddInt64(&r.totalInputTokens, resp.Usage.PromptTokens) - atomic.AddInt64(&r.totalOutputTokens, resp.Usage.CompletionTokens) - atomic.AddInt64(&r.totalCacheReadTokens, resp.Usage.CacheReadTokens) - atomic.AddInt64(&r.totalCacheWriteTokens, resp.Usage.CacheWriteTokens) - } - } else { - rlRec.SetError(fmt.Errorf("re-location LLM call failed"), time.Since(rlStart)) + if !located && len(candidates) > 1 && r.deps.Template.CandidateReLocationTask != nil { + from := cm.Path + msgs := diff.BuildCandidateReLocationMessages(cm, candidates, r.deps.Template.CandidateReLocationTask) + ok := false + // Use the same prompt budget gate as addNextMessage before + // issuing this extra re-location call. + if CountMessagesTokens(msgs) <= PromptTokenLimit(r.deps.Template.MaxTokens) { + var resp *llm.ChatResponse + ok, resp = r.runReLocationTask(rctx, cm.Path, msgs, func(reqCtx context.Context) (bool, *llm.ChatResponse) { + return diff.ReLocateCommentCandidate(reqCtx, cm, candidates, r.deps.LLMClient, msgs, r.deps.Model, r.deps.Template.CompletionTokenLimit()) + }, "re-location candidate selection failed") + for retry := 0; !ok && resp != nil && retry < 2 && needsCandidateReLocationRetry(resp.Content(), candidates); retry++ { + nextMsgs := candidateReLocationMessagesForRetry(msgs, resp) + if CountMessagesTokens(nextMsgs) > PromptTokenLimit(r.deps.Template.MaxTokens) { + break } + msgs = nextMsgs + ok, resp = r.runReLocationTask(rctx, cm.Path, msgs, func(reqCtx context.Context) (bool, *llm.ChatResponse) { + return diff.ReLocateCommentCandidate(reqCtx, cm, candidates, r.deps.LLMClient, msgs, r.deps.Model, r.deps.Template.CompletionTokenLimit()) + }, "re-location candidate selection retry failed") } + located = ok + } + if ok && cm.Path != from { + r.RecordWarning("comment_refiled", cm.Path, fmt.Sprintf( + "comment filed against %s describes code in %s; re-filed", from, cm.Path)) + } + } + if d != nil { + if !located && len(candidates) == 0 && r.deps.Template.ReLocationTask != nil { + msgs := diff.BuildReLocationMessages(cm, d, r.deps.Template.ReLocationTask) + r.runReLocationTask(rctx, cm.Path, msgs, func(reqCtx context.Context) (bool, *llm.ChatResponse) { + return diff.ReLocateComment(reqCtx, cm, d, r.deps.LLMClient, msgs, r.deps.Model, r.deps.Template.CompletionTokenLimit()) + }, "re-location LLM call failed") } } r.deps.CommentCollector.Add(*cm) diff --git a/internal/llmloop/loop_execute_more_test.go b/internal/llmloop/loop_execute_more_test.go index 7c938ea93..9065388e4 100644 --- a/internal/llmloop/loop_execute_more_test.go +++ b/internal/llmloop/loop_execute_more_test.go @@ -19,10 +19,16 @@ import ( // letting tests drive the full main loop turn by turn. type scriptedLLMClient struct { responses []*llm.ChatResponse + requests []llm.ChatRequest calls int } -func (s *scriptedLLMClient) CompletionsWithCtx(_ context.Context, _ llm.ChatRequest) (*llm.ChatResponse, error) { +func ptr[T any](v T) *T { + return &v +} + +func (s *scriptedLLMClient) CompletionsWithCtx(_ context.Context, req llm.ChatRequest) (*llm.ChatResponse, error) { + s.requests = append(s.requests, req) if s.calls >= len(s.responses) { return s.responses[len(s.responses)-1], nil } @@ -68,6 +74,13 @@ func codeCommentResponse(reasoning, content string) *llm.ChatResponse { } } +func testCandidateReLocationTask() *template.LlmConversation { + return &template.LlmConversation{Messages: []template.ChatMessage{ + {Role: "system", Content: "select candidate"}, + {Role: "user", Content: "{suggestion_content}\n{existing_code}\n{suggestion_code}\n{thinking}\n{candidates}"}, + }} +} + // TestRunPerFile_BackfillsThinkingFromReasoningContent verifies the full // wiring: the model's native reasoning_content on a tool-calling turn is // backfilled into the comment's thinking, while the turn's assistant @@ -266,6 +279,200 @@ func TestExecuteToolCall_CodeCommentDiffResolved(t *testing.T) { } } +func TestExecuteToolCall_CodeCommentAmbiguousSameFileUsesCandidateRelocation(t *testing.T) { + collector := tool.NewCommentCollector() + reg := tool.NewRegistry() + reg.Register(&tool.CodeCommentProvider{Collector: collector}) + reg.Freeze() + + client := &fakeClient{responses: []*llm.ChatResponse{{ + Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: ptr(`{"candidate_id":2}`)}}}, + Usage: &llm.UsageInfo{PromptTokens: 3, CompletionTokens: 1}, + }}} + d := &model.Diff{ + NewPath: "main.go", + Diff: `@@ -10,6 +10,10 @@ + func first() { ++ beforeFirst() ++ target() ++ afterFirst() + } +@@ -40,6 +44,10 @@ + func second() { ++ beforeSecond() ++ target() ++ afterSecond() + } +`, + } + r := NewRunner(Deps{ + LLMClient: client, + Model: "test-model", + Template: template.Template{MaxToolRequestTimes: 5, MaxTokens: 1000000, CandidateReLocationTask: testCandidateReLocationTask()}, + Tools: reg, + CommentCollector: collector, + DiffLookup: func(string) *model.Diff { return d }, + AllDiffs: func() []model.Diff { return []model.Diff{*d} }, + Session: session.New(t.TempDir(), "main", "test-model", session.SessionOptions{ReviewMode: "diff"}), + }) + + cp := r.executeToolCall(context.Background(), "main.go", llm.ToolCall{ + Function: llm.FunctionCall{ + Name: tool.CodeComment.Name(), + Arguments: `{"comments":[{"content":"the second branch is wrong","existing_code":"target()"}]}`, + }, + }, &session.TaskRecord{}, "") + if cp.Data != tool.CommentSucceed { + t.Fatalf("cp.Data = %q, want CommentSucceed", cp.Data) + } + comments := collector.Comments() + if len(comments) != 1 { + t.Fatalf("collected %d comments, want 1", len(comments)) + } + if comments[0].StartLine != 46 || comments[0].EndLine != 46 { + t.Fatalf("comment lines = %d-%d, want 46-46", comments[0].StartLine, comments[0].EndLine) + } + if client.calls != 1 { + t.Fatalf("LLM calls = %d, want 1 candidate re-location call", client.calls) + } + if got := client.requests[0].Messages[1].ExtractText(); !strings.Contains(got, "beforeSecond()") { + t.Fatalf("candidate prompt missing context: %s", got) + } +} + +func TestExecuteToolCall_CodeCommentPrefersUniqueCurrentFileMatch(t *testing.T) { + collector := tool.NewCommentCollector() + reg := tool.NewRegistry() + reg.Register(&tool.CodeCommentProvider{Collector: collector}) + reg.Freeze() + + client := &fakeClient{responses: []*llm.ChatResponse{{ + Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: ptr(`{"candidate_id":2}`)}}}, + }}} + current := model.Diff{ + NewPath: "current.go", + Diff: `@@ -10,3 +10,5 @@ + func current() { ++ target() + } +`, + } + other := model.Diff{ + NewPath: "other.go", + Diff: `@@ -20,3 +20,5 @@ + func other() { ++ target() + } +`, + } + r := NewRunner(Deps{ + LLMClient: client, + Model: "test-model", + Template: template.Template{MaxToolRequestTimes: 5, MaxTokens: 1000000, CandidateReLocationTask: testCandidateReLocationTask()}, + Tools: reg, + CommentCollector: collector, + DiffLookup: func(path string) *model.Diff { + if path == "current.go" { + return ¤t + } + return nil + }, + AllDiffs: func() []model.Diff { return []model.Diff{current, other} }, + Session: session.New(t.TempDir(), "main", "test-model", session.SessionOptions{ReviewMode: "diff"}), + }) + + cp := r.executeToolCall(context.Background(), "current.go", llm.ToolCall{ + Function: llm.FunctionCall{ + Name: tool.CodeComment.Name(), + Arguments: `{"comments":[{"content":"issue","existing_code":"target()"}]}`, + }, + }, &session.TaskRecord{}, "") + if cp.Data != tool.CommentSucceed { + t.Fatalf("cp.Data = %q, want CommentSucceed", cp.Data) + } + comments := collector.Comments() + if len(comments) != 1 { + t.Fatalf("collected %d comments, want 1", len(comments)) + } + if comments[0].Path != "current.go" || comments[0].StartLine != 11 || comments[0].EndLine != 11 { + t.Fatalf("comment location = %s:%d-%d, want current.go:11-11", comments[0].Path, comments[0].StartLine, comments[0].EndLine) + } + if client.calls != 0 { + t.Fatalf("LLM calls = %d, want no candidate re-location call", client.calls) + } +} + +func TestExecuteToolCall_CodeCommentCandidateRelocationRetriesTwiceStrictly(t *testing.T) { + collector := tool.NewCommentCollector() + reg := tool.NewRegistry() + reg.Register(&tool.CodeCommentProvider{Collector: collector}) + reg.Freeze() + + client := &fakeClient{responses: []*llm.ChatResponse{ + {Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: ptr("Candidate 2 is not the correct location.")}}}}, + {Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: ptr(`{"candidate_id":99}`)}}}}, + {Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: ptr(`{"candidate_id":2}`)}}}}, + }} + d := &model.Diff{ + NewPath: "main.go", + Diff: `@@ -10,6 +10,10 @@ + func first() { ++ beforeFirst() ++ target() ++ afterFirst() + } +@@ -40,6 +44,10 @@ + func second() { ++ beforeSecond() ++ target() ++ afterSecond() + } +`, + } + r := NewRunner(Deps{ + LLMClient: client, + Model: "test-model", + Template: template.Template{MaxToolRequestTimes: 5, MaxTokens: 1000000, CandidateReLocationTask: testCandidateReLocationTask()}, + Tools: reg, + CommentCollector: collector, + DiffLookup: func(string) *model.Diff { return d }, + AllDiffs: func() []model.Diff { return []model.Diff{*d} }, + Session: session.New(t.TempDir(), "main", "test-model", session.SessionOptions{ReviewMode: "diff"}), + }) + + cp := r.executeToolCall(context.Background(), "main.go", llm.ToolCall{ + Function: llm.FunctionCall{ + Name: tool.CodeComment.Name(), + Arguments: `{"comments":[{"content":"the second branch is wrong","existing_code":"target()"}]}`, + }, + }, &session.TaskRecord{}, "") + if cp.Data != tool.CommentSucceed { + t.Fatalf("cp.Data = %q, want CommentSucceed", cp.Data) + } + comments := collector.Comments() + if len(comments) != 1 { + t.Fatalf("collected %d comments, want 1", len(comments)) + } + if comments[0].StartLine != 46 || comments[0].EndLine != 46 { + t.Fatalf("comment lines = %d-%d, want 46-46", comments[0].StartLine, comments[0].EndLine) + } + if client.calls != 3 { + t.Fatalf("LLM calls = %d, want initial call plus 2 retries", client.calls) + } + if len(client.requests) != 3 { + t.Fatalf("recorded requests = %d, want 3", len(client.requests)) + } + if got := len(client.requests[0].Messages); got != 2 { + t.Fatalf("first request messages = %d, want 2", got) + } + if got := len(client.requests[1].Messages); got != 4 { + t.Fatalf("first retry messages = %d, want 4", got) + } + if got := len(client.requests[2].Messages); got != 6 { + t.Fatalf("second retry messages = %d, want 6", got) + } +} + // TestExecuteToolCall_CodeCommentThinkingBackfill covers the thinking backfill: // when the current turn carries reasoning content, comments without an explicit // thinking get the turn reasoning; explicit thinking wins.