From 375f64cd6f67a48a135da83cfcf59c52669a09cb Mon Sep 17 00:00:00 2001 From: ydflow <389051175@qq.com> Date: Tue, 20 Oct 2026 17:50:03 +0800 Subject: [PATCH] fix(tool): report an empty code_comment batch instead of success (#1773) Co-authored-by: ydflow <314143294+ydflow@users.noreply.github.com> Co-authored-by: Kite <254839944+lizhengfeng101@users.noreply.github.com> --- internal/llmloop/loop.go | 4 +++ internal/llmloop/loop_test.go | 38 +++++++++++++++++++++++++++++ internal/tool/code_comment.go | 6 +++++ internal/tool/code_comment_test.go | 39 ++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+) diff --git a/internal/llmloop/loop.go b/internal/llmloop/loop.go index 9f0e72d1a..131a224ed 100644 --- a/internal/llmloop/loop.go +++ b/internal/llmloop/loop.go @@ -731,6 +731,10 @@ func (r *Runner) executeToolCall(ctx context.Context, taskKey string, call llm.T // repair would absorb an unbounded number of them unobserved. r.RecordWarning("comment_args_repaired", taskKey, repair.Message()) } + // Report failure so the model can retry instead of assuming its findings were saved. + if errMsg == "" && len(comments) == 0 { + errMsg = tool.CommentNoneUsableMsg + } if errMsg != "" { dur := time.Since(startTime) toolErr := fmt.Errorf("%s", errMsg) diff --git a/internal/llmloop/loop_test.go b/internal/llmloop/loop_test.go index fae2af0a9..681e0ad44 100644 --- a/internal/llmloop/loop_test.go +++ b/internal/llmloop/loop_test.go @@ -651,6 +651,44 @@ func TestExecuteToolCall_CodeCommentUsesPerItemPath(t *testing.T) { } } +func TestExecuteToolCall_CodeCommentWithNoUsableEntryIsReported(t *testing.T) { + collector := tool.NewCommentCollector() + reg := tool.NewRegistry() + reg.Register(&tool.CodeCommentProvider{Collector: collector}) + reg.Freeze() + + r := NewRunner(Deps{ + Tools: reg, + CommentCollector: collector, + }) + + argsJSON, err := json.Marshal(map[string]any{ + "comments": []any{ + map[string]any{"path": "main.go", "message": "nil dereference"}, + }, + }) + if err != nil { + t.Fatal(err) + } + + cp := r.executeToolCall(context.Background(), "main.go", llm.ToolCall{ + Function: llm.FunctionCall{ + Name: "code_comment", + Arguments: string(argsJSON), + }, + }, nil, "") + + if cp.Data != tool.CommentNoneUsableMsg { + t.Fatalf("result = %q, want %q", cp.Data, tool.CommentNoneUsableMsg) + } + if len(collector.Comments()) != 0 { + t.Fatalf("collected %d comments, want 0", len(collector.Comments())) + } + if failures := r.ToolFailures(); len(failures) != 1 || failures[0].ToolName != "code_comment" { + t.Fatalf("ToolFailures() = %+v, want one code_comment failure", failures) + } +} + func graceRoundCommentResponse() *llm.ChatResponse { content := "" return &llm.ChatResponse{ diff --git a/internal/tool/code_comment.go b/internal/tool/code_comment.go index f26555d61..6b7a3ad5d 100644 --- a/internal/tool/code_comment.go +++ b/internal/tool/code_comment.go @@ -47,6 +47,9 @@ var validCodeCommentSeverities = map[string]struct{}{ codeCommentSeverityLow: {}, } +// CommentNoneUsableMsg reports a parsed batch with no usable comments. +const CommentNoneUsableMsg = "Error: none of the 'comments' entries could be filed; each entry needs a non-empty 'content' and a 'path' (or the reviewed file's path). Re-send the finding with those fields." + // CodeCommentProvider submits review comments to the per-Agent CommentCollector. type CodeCommentProvider struct { Collector *CommentCollector @@ -63,6 +66,9 @@ func (p *CodeCommentProvider) Execute(_ context.Context, args map[string]any) (s if errMsg != "" { return errMsg, nil } + if len(comments) == 0 { + return CommentNoneUsableMsg, nil + } for i := range comments { p.Collector.Add(comments[i]) diff --git a/internal/tool/code_comment_test.go b/internal/tool/code_comment_test.go index 68cc9d7fe..d01c83a57 100644 --- a/internal/tool/code_comment_test.go +++ b/internal/tool/code_comment_test.go @@ -310,6 +310,45 @@ func TestCodeCommentProvider_Execute(t *testing.T) { } }) + t.Run("all entries dropped returns error message", func(t *testing.T) { + collector := NewCommentCollector() + p := &CodeCommentProvider{Collector: collector} + result, err := p.Execute(context.Background(), map[string]any{ + "comments": []any{ + map[string]any{"path": "main.go", "message": "nil dereference"}, + }, + }) + if err != nil { + t.Fatal(err) + } + if result != CommentNoneUsableMsg { + t.Errorf("result = %q, want %q", result, CommentNoneUsableMsg) + } + if len(collector.Comments()) != 0 { + t.Errorf("collector has %d comments, want 0", len(collector.Comments())) + } + }) + + t.Run("partially dropped batch still succeeds", func(t *testing.T) { + collector := NewCommentCollector() + p := &CodeCommentProvider{Collector: collector} + result, err := p.Execute(context.Background(), map[string]any{ + "comments": []any{ + map[string]any{"content": "real finding", "path": "main.go"}, + map[string]any{"path": "main.go", "message": "dropped: no content"}, + }, + }) + if err != nil { + t.Fatal(err) + } + if result != CommentSucceed { + t.Errorf("result = %q, want %q", result, CommentSucceed) + } + if len(collector.Comments()) != 1 { + t.Errorf("collector has %d comments, want 1", len(collector.Comments())) + } + }) + t.Run("nil collector returns error message", func(t *testing.T) { p := &CodeCommentProvider{Collector: nil} result, err := p.Execute(context.Background(), map[string]any{