Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions internal/llm/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,14 @@ func (r *ChatResponse) ToolCalls() []ToolCall {
return r.Choices[0].Message.ToolCalls
}

// ReasoningContent extracts the reasoning content of the first choice, if any.
func (r *ChatResponse) ReasoningContent() string {
if len(r.Choices) == 0 {
return ""
}
return r.Choices[0].Message.ReasoningContent
}

// ToolDef defines a tool/function available to the model.
type ToolDef struct {
Type string `json:"type"`
Expand Down
17 changes: 15 additions & 2 deletions internal/llmloop/loop.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,12 @@ func (r *Runner) RunPerFile(ctx context.Context, messages []llm.Message, newPath
taskCompleted := false
hasValidResult := false

// Capture the model's native reasoning content for this turn. Models
// without a reasoning channel leave it empty.
// Reasoning is turn-level, so all tool calls in this turn share it.
thinking := resp.ReasoningContent()
for _, call := range calls {
cp := r.executeToolCall(ctx, newPath, call, rec)
cp := r.executeToolCall(ctx, newPath, call, rec, thinking)
if cp.Failed {
return false, StopNone, fmt.Errorf("task failed: %s", cp.Data)
} else if cp.Completed {
Expand Down Expand Up @@ -307,7 +311,7 @@ func (r *Runner) RunPerFile(ctx context.Context, messages []llm.Message, newPath
// records the result in session history. code_comment handling includes
// optional async dispatch through CommentWorkerPool plus line-number
// resolution / re-location.
func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.ToolCall, rec *session.TaskRecord) tool.TaskCheckpoint {
func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.ToolCall, rec *session.TaskRecord, thinking string) tool.TaskCheckpoint {
t := tool.OfName(call.Function.Name)

if !t.IsKnown() {
Expand Down Expand Up @@ -398,6 +402,15 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T
return tool.Of(errMsg)
}

// Batched comments share the turn's thinking.
if thinking != "" {
for i := range comments {
if comments[i].Thinking == "" {
comments[i].Thinking = thinking
}
}
}

resolveAndCollect := func(rctx context.Context) {
for i := range comments {
cm := &comments[i]
Expand Down
173 changes: 170 additions & 3 deletions internal/llmloop/loop_execute_more_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,115 @@ import (
"strings"
"testing"

"github.com/alibaba/open-code-review/internal/config/template"
"github.com/alibaba/open-code-review/internal/llm"
"github.com/alibaba/open-code-review/internal/model"
"github.com/alibaba/open-code-review/internal/session"
"github.com/alibaba/open-code-review/internal/tool"
)

// scriptedLLMClient returns a fixed sequence of responses, one per call,
// letting tests drive the full main loop turn by turn.
type scriptedLLMClient struct {
responses []*llm.ChatResponse
calls int
}

func (s *scriptedLLMClient) CompletionsWithCtx(_ context.Context, _ llm.ChatRequest) (*llm.ChatResponse, error) {
if s.calls >= len(s.responses) {
return s.responses[len(s.responses)-1], nil
}
resp := s.responses[s.calls]
s.calls++
return resp, nil
}

// newThinkingTestRunner builds a Runner wired for a full RunPerFile pass:
// a scripted LLM client, the code_comment tool, and a comment collector.
func newThinkingTestRunner(t *testing.T, client llm.LLMClient) (*Runner, *tool.CommentCollector) {
t.Helper()
collector := tool.NewCommentCollector()
reg := tool.NewRegistry()
reg.Register(&tool.CodeCommentProvider{Collector: collector})
reg.Freeze()
r := NewRunner(Deps{
LLMClient: client,
Model: "test-model",
Template: template.Template{MaxToolRequestTimes: 5, MaxTokens: 1000000},
Tools: reg,
CommentCollector: collector,
MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: tool.CodeComment.Name()}}},
Session: session.New(t.TempDir(), "main", "test-model", session.SessionOptions{ReviewMode: "diff"}),
})
return r, collector
}

func codeCommentResponse(reasoning, content string) *llm.ChatResponse {
return &llm.ChatResponse{
Choices: []llm.Choice{{Message: llm.ResponseMessage{
Content: &content,
ReasoningContent: reasoning,
ToolCalls: []llm.ToolCall{{
ID: "call_comment",
Type: "function",
Function: llm.FunctionCall{
Name: tool.CodeComment.Name(),
Arguments: `{"comments":[{"content":"issue","existing_code":"x"}]}`,
},
}},
}}},
}
}

// 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
// content is ignored.
func TestRunPerFile_BackfillsThinkingFromReasoningContent(t *testing.T) {
client := &scriptedLLMClient{responses: []*llm.ChatResponse{
codeCommentResponse("native reasoning", "I'll now leave a comment on this file"),
taskDoneResponse(),
}}
r, collector := newThinkingTestRunner(t, client)

ok, _, err := r.RunPerFile(context.Background(), []llm.Message{msg("user", "review")}, "file.go")
if err != nil || !ok {
t.Fatalf("RunPerFile = (%v, err %v), want completed", ok, err)
}

comments := collector.Comments()
if len(comments) != 1 {
t.Fatalf("collected %d comments, want 1", len(comments))
}
if comments[0].Thinking != "native reasoning" {
t.Errorf("comment Thinking = %q, want native reasoning_content", comments[0].Thinking)
}
}

// TestRunPerFile_NoFallbackToContent is a regression test: when a turn has
// assistant content but no reasoning_content, the comment thinking must stay
// empty. It fails if the removed `thinking = content` fallback returns.
func TestRunPerFile_NoFallbackToContent(t *testing.T) {
client := &scriptedLLMClient{responses: []*llm.ChatResponse{
codeCommentResponse("", "I'll now leave a comment on this file"),
taskDoneResponse(),
}}
r, collector := newThinkingTestRunner(t, client)

ok, _, err := r.RunPerFile(context.Background(), []llm.Message{msg("user", "review")}, "file.go")
if err != nil || !ok {
t.Fatalf("RunPerFile = (%v, err %v), want completed", ok, err)
}

comments := collector.Comments()
if len(comments) != 1 {
t.Fatalf("collected %d comments, want 1", len(comments))
}
if comments[0].Thinking != "" {
t.Errorf("comment Thinking = %q, want empty (no content fallback)", comments[0].Thinking)
}
}

// TestExecuteToolCall_TaskDone covers every branch of the task_done handling:
// argument parse error, missing state (implicit completion), non-string state,
// explicit DONE / FAILED, and an unrecognized state value.
Expand All @@ -27,7 +130,7 @@ func TestExecuteToolCall_TaskDone(t *testing.T) {
call := func(args string) tool.TaskCheckpoint {
return newRunner().executeToolCall(context.Background(), "file.go", llm.ToolCall{
Function: llm.FunctionCall{Name: tool.TaskDone.Name(), Arguments: args},
}, nil)
}, nil, "")
}

t.Run("parse error", func(t *testing.T) {
Expand Down Expand Up @@ -96,7 +199,7 @@ func TestExecuteToolCall_CodeCommentAsyncPool(t *testing.T) {
Name: tool.CodeComment.Name(),
Arguments: `{"comments":[{"content":"issue","existing_code":"foo"}]}`,
},
}, rec)
}, rec, "")

if cp.Data != tool.CommentSucceed {
t.Fatalf("cp.Data = %q, want CommentSucceed", cp.Data)
Expand Down Expand Up @@ -144,7 +247,7 @@ func TestExecuteToolCall_CodeCommentDiffResolved(t *testing.T) {
Name: tool.CodeComment.Name(),
Arguments: `{"comments":[{"content":"issue","existing_code":"foo bar"}]}`,
},
}, rec)
}, rec, "")

if cp.Data != tool.CommentSucceed {
t.Fatalf("cp.Data = %q, want CommentSucceed", cp.Data)
Expand All @@ -162,3 +265,67 @@ func TestExecuteToolCall_CodeCommentDiffResolved(t *testing.T) {
t.Errorf("comment StartLine = %d, want 2 (resolved from file content)", comments[0].StartLine)
}
}

// 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.
func TestExecuteToolCall_CodeCommentThinkingBackfill(t *testing.T) {
collector := tool.NewCommentCollector()
reg := tool.NewRegistry()
reg.Register(&tool.CodeCommentProvider{Collector: collector})
reg.Freeze()

r := NewRunner(Deps{Tools: reg, CommentCollector: collector})

cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{
Function: llm.FunctionCall{
Name: tool.CodeComment.Name(),
Arguments: `{"comments":[` +
`{"content":"a","existing_code":"x"},` +
`{"content":"b","existing_code":"y","thinking":"explicit"}]}`,
},
}, nil, "turn reasoning")

if cp.Data != tool.CommentSucceed {
t.Fatalf("cp.Data = %q, want CommentSucceed", cp.Data)
}
comments := collector.Comments()
if len(comments) != 2 {
t.Fatalf("collected %d comments, want 2", len(comments))
}
if comments[0].Thinking != "turn reasoning" {
t.Errorf("comments[0].Thinking = %q, want backfilled turn reasoning", comments[0].Thinking)
}
if comments[1].Thinking != "explicit" {
t.Errorf("comments[1].Thinking = %q, want explicit thinking preserved", comments[1].Thinking)
}
}

// TestExecuteToolCall_CodeCommentNoReasoning verifies comments keep an empty
// thinking when the turn has no reasoning content.
func TestExecuteToolCall_CodeCommentNoReasoning(t *testing.T) {
collector := tool.NewCommentCollector()
reg := tool.NewRegistry()
reg.Register(&tool.CodeCommentProvider{Collector: collector})
reg.Freeze()

r := NewRunner(Deps{Tools: reg, CommentCollector: collector})

cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{
Function: llm.FunctionCall{
Name: tool.CodeComment.Name(),
Arguments: `{"comments":[{"content":"a","existing_code":"x"}]}`,
},
}, nil, "")

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].Thinking != "" {
t.Errorf("comments[0].Thinking = %q, want empty", comments[0].Thinking)
}
}
10 changes: 5 additions & 5 deletions internal/llmloop/loop_execute_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func TestExecuteToolCall_DynamicNotRegistered(t *testing.T) {

cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{
Function: llm.FunctionCall{Name: "totally_unknown", Arguments: `{}`},
}, nil)
}, nil, "")

if cp.Data != tool.NotAvailableMsg {
t.Errorf("cp.Data = %q, want NotAvailableMsg", cp.Data)
Expand All @@ -52,7 +52,7 @@ func TestExecuteToolCall_DynamicExecuteError(t *testing.T) {

cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{
Function: llm.FunctionCall{Name: "dyn_fail", Arguments: `{}`},
}, nil)
}, nil, "")

if !strings.Contains(cp.Data, "Error executing tool dyn_fail") {
t.Errorf("cp.Data = %q, want execute-error message", cp.Data)
Expand All @@ -71,7 +71,7 @@ func TestExecuteToolCall_DynamicSuccessRecordsResult(t *testing.T) {
rec := &session.TaskRecord{}
cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{
Function: llm.FunctionCall{Name: "dyn_ok", Arguments: `{"k":"v"}`},
}, rec)
}, rec, "")

if cp.Data != "ok" {
t.Errorf("cp.Data = %q, want ok", cp.Data)
Expand All @@ -93,7 +93,7 @@ func TestExecuteToolCall_KnownToolNotRegistered(t *testing.T) {

cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{
Function: llm.FunctionCall{Name: tool.FileRead.Name(), Arguments: `{"path":"x"}`},
}, nil)
}, nil, "")

if cp.Data != tool.NotAvailableMsg {
t.Errorf("cp.Data = %q, want NotAvailableMsg", cp.Data)
Expand Down Expand Up @@ -138,7 +138,7 @@ func TestExecuteToolCall_DynamicParseError(t *testing.T) {

cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{
Function: llm.FunctionCall{Name: "dyn_ok", Arguments: `{bad`},
}, nil)
}, nil, "")

if !strings.Contains(cp.Data, "Error parsing tool arguments for dyn_ok") {
t.Errorf("cp.Data = %q, want parse-error message", cp.Data)
Expand Down
4 changes: 2 additions & 2 deletions internal/llmloop/loop_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ func TestExecuteToolCall_ArgumentsEdgeCases(t *testing.T) {
Name: tt.toolName,
Arguments: tt.arguments,
},
}, nil)
}, nil, "")

if tt.wantContains != "" && !strings.Contains(cp.Data, tt.wantContains) {
t.Errorf("cp.Data = %q, want substring %q", cp.Data, tt.wantContains)
Expand Down Expand Up @@ -581,7 +581,7 @@ func TestExecuteToolCall_CodeCommentOverridesHallucinatedPath(t *testing.T) {
Name: "code_comment",
Arguments: string(argsJSON),
},
}, nil)
}, nil, "")
if cp.Data != tool.CommentSucceed {
t.Fatalf("unexpected result: %+v", cp)
}
Expand Down
7 changes: 5 additions & 2 deletions pages/src/content/docs/en/tools.md

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Russian site documentation is missing, use a translator to translate this to pages/scr/content/docs/ru/tools.md

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synced it.

Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,11 @@ is optional but encouraged. `path` is a top-level optional override —
if omitted, the agent injects the file currently under review. The
agent also injects `path` automatically when the model leaves it out, so
the model rarely needs to set it explicitly. `thinking` (per-comment)
captures the model's reasoning and is preserved on the comment but not
shown in the final review output.
captures the model's reasoning and is preserved on the comment; OCR
backfills it from the reasoning content the model emits on the current
turn (it stays empty when the model emits none), and includes it in
the JSON output (the terminal output does not
render it).

> **`thinking` is a runtime-only field.** OCR parses and stores it, but
> it is deliberately **not** listed in the `code_comment` schema
Expand Down
4 changes: 2 additions & 2 deletions pages/src/content/docs/ja/tools.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ OCR が行番号を自動計算できるようにします。
`comments` は配列なので、モデルは 1 回のツール呼び出しで複数のコメントを発行できます。`content` と
`existing_code` は必須です。`suggestion_code` は任意ですが、提供が推奨されます。`path` はトップレベルの任意の上書きで——
省略すると、agent は現在レビュー中のファイルを注入します。モデルが省略しても agent が自動的に `path` を注入するため、
モデルが明示的に設定する必要はほとんどありません。`thinking`(コメントごと)はモデルの推論を捕捉し、コメントに保持されますが、
最終的なレビュー出力には表示されません
モデルが明示的に設定する必要はほとんどありません。`thinking`(コメントごと)はモデルの推論を捕捉し、コメントに保持されます。
OCR はモデルが現在のターンで出力した推論内容で自動的に補完します(推論内容がない場合は空のまま)。JSON 出力に含めます(ターミナル出力には表示されません)

> **`thinking` はランタイム専用フィールドです。** OCR はこれを解析して保存しますが、モデルに渡す
> `code_comment` schema には意図的に**含めていません**(`tools.json` には `content`、
Expand Down
5 changes: 4 additions & 1 deletion pages/src/content/docs/ru/tools.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,10 @@ JSON-файл той же структуры, что и встроенный. Т
проверяемый в данный момент файл. Агент также автоматически добавляет `path`,
если модель его не указала, поэтому модели редко требуется задавать это поле
явно. Поле `thinking` для каждого комментария содержит рассуждение модели и
сохраняется вместе с комментарием, но не показывается в итоговом выводе ревью.
сохраняется вместе с комментарием; OCR заполняет его из содержимого
рассуждений (reasoning content), которое модель выводит в текущем ходе
(поле остаётся пустым, если модель ничего не выводит), и включает его в
JSON-вывод (в терминальном выводе оно не отображается).

> **`thinking` существует только во время выполнения.** OCR разбирает и
> сохраняет его, однако намеренно **не** включает в схему `code_comment`,
Expand Down
4 changes: 2 additions & 2 deletions pages/src/content/docs/zh/tools.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ OCR 自动计算行号。
`comments` 是数组,因此模型可以在一次工具调用中发出多条评论。`content` 和
`existing_code` 必需;`suggestion_code` 可选但建议提供。`path` 是顶层可选覆盖——
省略时,agent 会注入当前评审的文件。即便模型省略,agent 也会自动注入 `path`,因此
模型极少需要显式设置。`thinking`(按评论)捕获模型推理,保留在评论上,但不会
在最终评审输出中显示
模型极少需要显式设置。`thinking`(按评论)捕获模型推理,保留在评论上;OCR 会用
模型当轮输出的推理内容自动回填(模型未输出推理内容时保持为空),并包含在 JSON 输出中(终端输出不渲染)

> **`thinking` 是运行时专属字段。** OCR 会解析并存储它,但有意**不**把它列入
> 给模型的 `code_comment` schema(`tools.json` 中只有 `content`、
Expand Down
Loading