Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
18 changes: 16 additions & 2 deletions internal/llmloop/loop.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,14 @@ func (r *Runner) RunPerFile(ctx context.Context, messages []llm.Message, newPath
taskCompleted := false
hasValidResult := false

// Prefer the model's native reasoning content; fall back to the
// assistant message of this turn for models that do not expose it.

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.

Suggested change
// Prefer the model's native reasoning content; fall back to the
// assistant message of this turn for models that do not expose it.
// Capture the model's native reasoning content for this turn. Models
// without a reasoning channel leave it empty.

thinking := resp.ReasoningContent()
if thinking == "" {
thinking = content
}

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.

Suggested change
if thinking == "" {
thinking = content
}

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.

Capture the model's native reasoning content for this turn.

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.

Done

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 +313,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 +404,14 @@ func (r *Runner) executeToolCall(ctx context.Context, newPath string, call llm.T
return tool.Of(errMsg)
}

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
70 changes: 67 additions & 3 deletions internal/llmloop/loop_execute_more_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,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 +96,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 +144,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 +162,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
@@ -1,3 +1,3 @@
---
title: Tools
sidebar:
Expand Down Expand Up @@ -89,8 +89,11 @@
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 (or the turn's message when there is none), and includes it in
the JSON output (the terminal output does not
render it).
Comment on lines -92 to +96

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.

If you applied the suggestion here: https://github.com/alibaba/open-code-review/pull/773/changes#r3740773775
This means you have to likely update the documentation here.


> **`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 出力に含めます(ターミナル出力には表示されません)

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.

ditto


> **`thinking` はランタイム専用フィールドです。** OCR はこれを解析して保存しますが、モデルに渡す
> `code_comment` schema には意図的に**含めていません**(`tools.json` には `content`、
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 输出中(终端输出不渲染)

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.

ditto


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