diff --git a/cmd/opencodereview/budget_output_test.go b/cmd/opencodereview/budget_output_test.go index 2c2ca0e6..810151c3 100644 --- a/cmd/opencodereview/budget_output_test.go +++ b/cmd/opencodereview/budget_output_test.go @@ -45,7 +45,7 @@ func TestEmitRunResult_JSONBudgetStopIsPartial(t *testing.T) { }, } got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -96,7 +96,7 @@ func TestEmitRunResult_JSONBudgetDoesNotOverrideLegacyStatus(t *testing.T) { budgetExceeded: true, } got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -124,7 +124,7 @@ func TestEmitRunResult_JSONNoBudgetIsSuccess(t *testing.T) { totalTokens: 15, } got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -249,7 +249,7 @@ func TestEmitRunResult_BudgetExceededFalseOmittedFromJSON(t *testing.T) { totalTokens: 10, } got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, []model.LlmComment{}, time.Now(), "json", "developer", nil, nil, nil) + err := emitRunResult(context.Background(), ag, []model.LlmComment{}, time.Now(), "json", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/cmd/opencodereview/emit_run_result_test.go b/cmd/opencodereview/emit_run_result_test.go index ce4fb1e4..0ac68fe8 100644 --- a/cmd/opencodereview/emit_run_result_test.go +++ b/cmd/opencodereview/emit_run_result_test.go @@ -91,7 +91,7 @@ func TestEmitRunResult_JSONNoFiles(t *testing.T) { ag := &mockResultProvider{filesReviewed: 0} identity := &jsonLLMIdentity{Provider: "anthropic", Model: "claude-opus-4-6"} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, identity, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, identity, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -112,7 +112,7 @@ func TestEmitRunResult_JSONLLMIdentityNamedProvider(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1} identity := &jsonLLMIdentity{Provider: "anthropic", Model: "claude-opus-4-6"} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, identity, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, identity, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -129,7 +129,7 @@ func TestEmitRunResult_JSONLLMIdentityOmitsUnknownProvider(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1} identity := &jsonLLMIdentity{Model: "gpt-5-codex"} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, identity, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, identity, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -156,7 +156,7 @@ func TestEmitRunResult_JSONUsesManifestTerminalState(t *testing.T) { }, } got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -184,7 +184,7 @@ func TestEmitRunResult_JSONUsesManifestTerminalState(t *testing.T) { func TestEmitRunResult_JSONSkippedIncludesManifest(t *testing.T) { ag := &mockResultProvider{manifest: mockManifest(session.StateSkipped)} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -239,7 +239,7 @@ func TestEmitRunResult_JSONManifestMatchesPersistedSessionEnd(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, sessionID: sh.SessionID, manifest: sh.FinalManifest()} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -302,7 +302,7 @@ func TestEmitRunResult_JSONWithComments(t *testing.T) { } comments := []model.LlmComment{{Path: "main.go", Content: "fix", StartLine: 1, EndLine: 2}} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, comments, time.Now(), "json", "developer", nil, nil, nil) + err := emitRunResult(context.Background(), ag, comments, time.Now(), "json", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -331,7 +331,7 @@ func TestEmitRunResult_JSONWithResumeInfo(t *testing.T) { }, } got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -348,7 +348,7 @@ func TestEmitRunResult_JSONWithResumeInfo(t *testing.T) { func TestEmitRunResult_TextNoComments(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -361,7 +361,7 @@ func TestEmitRunResult_TextNoComments(t *testing.T) { func TestEmitRunResult_TextPartialNeverLooksGood(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, manifest: mockManifest(session.StatePartial)} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", false, nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -379,7 +379,7 @@ func TestEmitRunResult_TextCompleteReportsFindingsAndWaived(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, manifest: manifest} comments := []model.LlmComment{{Path: "a.go", Content: "fix", StartLine: 1, EndLine: 1}} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, comments, time.Now(), "text", "developer", nil, nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, comments, time.Now(), "text", "developer", false, nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -393,7 +393,7 @@ func TestEmitRunResult_TextCompleteReportsFindingsAndWaived(t *testing.T) { func TestEmitRunResult_TextDoesNotPrintSuccessfulSessionHint(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, sessionID: "session-123"} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -407,7 +407,7 @@ func TestEmitRunResult_TextWithComments(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1} comments := []model.LlmComment{{Path: "a.go", Content: "rename", StartLine: 5, EndLine: 10}} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, comments, time.Now(), "text", "developer", nil, nil, nil) + err := emitRunResult(context.Background(), ag, comments, time.Now(), "text", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -426,7 +426,7 @@ func TestEmitRunResult_TextWithProjectSummary(t *testing.T) { projectSummary: "All tests pass, code quality is good.", } got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -443,7 +443,7 @@ func TestEmitRunResult_AgentTextRestoresQuiet(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1} q := newQuietHandle("text", "agent") got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "agent", q, nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "agent", false, q, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -463,7 +463,7 @@ func TestEmitRunResult_AgentJSONDoesNotRestore(t *testing.T) { } q := newQuietHandle("json", "agent") got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "agent", q, nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "agent", false, q, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -478,7 +478,7 @@ func TestEmitRunResult_AgentJSONDoesNotRestore(t *testing.T) { func TestEmitRunResult_NilQuietHandle(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "agent", nil, nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "agent", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -502,7 +502,7 @@ func TestEmitRunResult_JSONTraceIDFromContext(t *testing.T) { totalTokens: 15, } got := captureStdout(t, func() { - err := emitRunResult(ctx, ag, nil, time.Now(), "json", "developer", nil, nil, nil) + err := emitRunResult(ctx, ag, nil, time.Now(), "json", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -527,7 +527,7 @@ func TestEmitRunResult_JSONNoFilesTraceID(t *testing.T) { ag := &mockResultProvider{filesReviewed: 0} got := captureStdout(t, func() { - err := emitRunResult(ctx, ag, nil, time.Now(), "json", "developer", nil, nil, nil) + err := emitRunResult(ctx, ag, nil, time.Now(), "json", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -547,7 +547,7 @@ func TestEmitRunResult_JSONNoFilesTraceID(t *testing.T) { func TestEmitRunResult_JSONIncludesSessionID(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1, sessionID: "session-99"} got := captureStdout(t, func() { - err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil) + err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -570,7 +570,7 @@ func TestEmitRunResult_JSONCarriesRetryReport(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, manifest: mockManifest(session.StateComplete)} rep := retryReportFixture() got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, rep); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, nil, rep); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -593,7 +593,7 @@ func TestEmitRunResult_JSONCarriesRetryReport(t *testing.T) { func TestEmitRunResult_JSONOmitsRetryReportWhenNil(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, manifest: mockManifest(session.StateComplete)} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -611,7 +611,7 @@ func TestEmitRunResult_TextReportOrder(t *testing.T) { projectSummary: "PROJECT-SUMMARY-MARKER", } got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, retryReportFixture()); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", false, nil, nil, retryReportFixture()); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -631,7 +631,7 @@ func TestEmitRunResult_TextReportOrder(t *testing.T) { func TestEmitRunResult_TextOmitsReportWhenNil(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, manifest: mockManifest(session.StateComplete)} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", false, nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -645,7 +645,7 @@ func TestEmitRunResult_TextOmitsReportWhenNil(t *testing.T) { func TestEmitRunResult_JSONHasNoReportText(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, manifest: mockManifest(session.StateComplete)} got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, retryReportFixture()); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, nil, retryReportFixture()); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -710,7 +710,7 @@ func TestEmitRunResult_TextReportWithWarnings(t *testing.T) { warnings: []agent.AgentWarning{{Type: "subtask_error", File: "b.go", Message: "boom"}}, } got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil, nil, retryReportFixture()); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", false, nil, nil, retryReportFixture()); err != nil { t.Fatalf("emitRunResult: %v", err) } }) diff --git a/cmd/opencodereview/output.go b/cmd/opencodereview/output.go index 46378077..3fff01cd 100644 --- a/cmd/opencodereview/output.go +++ b/cmd/opencodereview/output.go @@ -19,13 +19,13 @@ import ( "github.com/alibaba/open-code-review/internal/suggestdiff" ) -func outputText(comments []model.LlmComment) { +func outputText(comments []model.LlmComment, showThinking bool) { if len(comments) == 0 { fmt.Println("No comments generated. Looks good to me.") return } for _, c := range comments { - renderComment(c) + renderComment(c, showThinking) } } @@ -63,11 +63,11 @@ func isSubtaskErrorType(warningType string) bool { return warningType == "subtask_error" || warningType == "scan_subtask_error" } -func outputTextWithWarnings(comments []model.LlmComment, warnings []agent.AgentWarning, manifest *session.RunManifest) { +func outputTextWithWarnings(comments []model.LlmComment, warnings []agent.AgentWarning, manifest *session.RunManifest, showThinking bool) { if manifest != nil { fmt.Println(manifestMessage(manifest, len(comments))) for _, c := range comments { - renderComment(c) + renderComment(c, showThinking) } } else if len(comments) == 0 { if hasSubtaskErrors(warnings) { @@ -77,7 +77,7 @@ func outputTextWithWarnings(comments []model.LlmComment, warnings []agent.AgentW } } else { for _, c := range comments { - renderComment(c) + renderComment(c, showThinking) } } for _, w := range warnings { @@ -88,7 +88,7 @@ func outputTextWithWarnings(comments []model.LlmComment, warnings []agent.AgentW } } -func renderComment(comment model.LlmComment) { +func renderComment(comment model.LlmComment, showThinking bool) { lines := buildDiffLines(comment) if len(lines) == 0 && comment.Content == "" { return @@ -115,6 +115,23 @@ func renderComment(comment model.LlmComment) { fmt.Println() } + // Render the LLM's reasoning after the content block and before the diff + // block, only when explicitly requested. Thinking stays hidden by default so + // the human-readable terminal stays compact; machine-readable JSON already + // carries it via omitempty (see #773). + if showThinking && comment.Thinking != "" { + // Account for prefix width to maintain consistent total line width + wrapped := wrapByRunes(sanitizeTerminal(comment.Thinking), 88) + for i, ln := range wrapped { + if i == 0 { + fmt.Printf("\033[2m> Thinking: %s\033[0m\n", ln) + } else { + fmt.Printf("\033[2m> %s\033[0m\n", ln) + } + } + fmt.Println() + } + if len(lines) > 0 { for _, dl := range lines { switch dl.Type { diff --git a/cmd/opencodereview/output_helpers_test.go b/cmd/opencodereview/output_helpers_test.go index f0976f3c..2f9fc938 100644 --- a/cmd/opencodereview/output_helpers_test.go +++ b/cmd/opencodereview/output_helpers_test.go @@ -416,7 +416,7 @@ func captureStderr(t *testing.T, fn func()) string { func TestOutputText_NoComments(t *testing.T) { got := captureStdout(t, func() { - outputText(nil) + outputText(nil, false) }) if !strings.Contains(got, "Looks good to me") { t.Errorf("expected 'Looks good to me', got %q", got) @@ -428,7 +428,7 @@ func TestOutputText_WithComments(t *testing.T) { {Path: "main.go", StartLine: 10, EndLine: 15, Content: "potential nil dereference"}, } got := captureStdout(t, func() { - outputText(comments) + outputText(comments, false) }) if !strings.Contains(got, "main.go") { t.Errorf("expected path in output, got %q", got) @@ -441,7 +441,7 @@ func TestOutputText_WithComments(t *testing.T) { func TestOutputTextWithWarnings_NoCommentsNoErrors(t *testing.T) { warnings := []agent.AgentWarning{{Type: "warning", File: "x.go", Message: "slow"}} got := captureStdout(t, func() { - outputTextWithWarnings(nil, warnings, nil) + outputTextWithWarnings(nil, warnings, nil, false) }) if !strings.Contains(got, "Looks good to me") { t.Errorf("expected 'Looks good to me', got %q", got) @@ -451,7 +451,7 @@ func TestOutputTextWithWarnings_NoCommentsNoErrors(t *testing.T) { func TestOutputTextWithWarnings_NoCommentsWithSubtaskError(t *testing.T) { warnings := []agent.AgentWarning{{Type: "subtask_error", File: "y.go", Message: "failed"}} got := captureStdout(t, func() { - outputTextWithWarnings(nil, warnings, nil) + outputTextWithWarnings(nil, warnings, nil, false) }) if !strings.Contains(got, "could not be reviewed") { t.Errorf("expected subtask error message, got %q", got) @@ -464,7 +464,7 @@ func TestOutputTextWithWarnings_WithComments(t *testing.T) { } warnings := []agent.AgentWarning{{Type: "info", File: "b.go", Message: "note"}} got := captureStdout(t, func() { - outputTextWithWarnings(comments, warnings, nil) + outputTextWithWarnings(comments, warnings, nil, false) }) if !strings.Contains(got, "a.go") { t.Errorf("expected comment path, got %q", got) @@ -476,7 +476,7 @@ func TestOutputTextWithWarnings_WithComments(t *testing.T) { func TestRenderComment_EmptyContentNoDiff(t *testing.T) { got := captureStdout(t, func() { - renderComment(model.LlmComment{Path: "skip.go", StartLine: 1, EndLine: 1, Content: "", ExistingCode: "", SuggestionCode: ""}) + renderComment(model.LlmComment{Path: "skip.go", StartLine: 1, EndLine: 1, Content: "", ExistingCode: "", SuggestionCode: ""}, false) }) if got != "" { t.Errorf("expected empty output for empty comment, got %q", got) @@ -485,7 +485,7 @@ func TestRenderComment_EmptyContentNoDiff(t *testing.T) { func TestRenderComment_ContentOnly(t *testing.T) { got := captureStdout(t, func() { - renderComment(model.LlmComment{Path: "file.go", StartLine: 5, EndLine: 10, Content: "consider renaming"}) + renderComment(model.LlmComment{Path: "file.go", StartLine: 5, EndLine: 10, Content: "consider renaming"}, false) }) if !strings.Contains(got, "file.go:5-10") { t.Errorf("expected path:line range, got %q", got) @@ -504,7 +504,7 @@ func TestRenderComment_WithDiff(t *testing.T) { Content: "rename var", ExistingCode: "old := 1\n", SuggestionCode: "new := 1\n", - }) + }, false) }) if !strings.Contains(got, "diff.go:1-2") { t.Errorf("expected path:line range, got %q", got) diff --git a/cmd/opencodereview/output_test.go b/cmd/opencodereview/output_test.go index 2742cb67..1ea8c504 100644 --- a/cmd/opencodereview/output_test.go +++ b/cmd/opencodereview/output_test.go @@ -63,7 +63,7 @@ func TestRenderComment_BadgeInline(t *testing.T) { Content: "Potential environment variable leak.", Category: "security", Severity: "high", - }) + }, false) }) if !strings.Contains(out, "[security ยท high]") { t.Errorf("expected badge in output, got:\n%s", out) @@ -108,3 +108,86 @@ func TestSanitizeTerminal(t *testing.T) { }) } } + +// TestRenderComment_ThinkingHiddenByDefault verifies a non-empty Thinking field +// is NOT rendered unless showThinking is true (backward-compatible text output). +func TestRenderComment_ThinkingHiddenByDefault(t *testing.T) { + got := captureStdout(t, func() { + renderComment(model.LlmComment{ + Path: "file.go", + StartLine: 5, + EndLine: 10, + Content: "consider renaming", + Thinking: "The variable name is misleading because it shadows package-level config.", + }, false) + }) + if strings.Contains(got, "Thinking:") { + t.Errorf("Thinking must be hidden by default, got:\n%s", got) + } +} + +// TestRenderComment_ThinkingRendered verifies the Thinking block renders after +// the content and before the diff, dimmed, with the first line prefixed +// "> Thinking: " and continuations "> ". +func TestRenderComment_ThinkingRendered(t *testing.T) { + got := captureStdout(t, func() { + renderComment(model.LlmComment{ + Path: "diff.go", + StartLine: 1, + EndLine: 2, + Content: "rename var", + Thinking: "The name collides with an existing helper in this package.", + ExistingCode: "old := 1\n", + SuggestionCode: "new := 1\n", + }, true) + }) + if !strings.Contains(got, "\033[2m> Thinking: ") { + t.Errorf("expected dim Thinking block, got:\n%q", got) + } + contentIdx := strings.Index(got, "rename var") + thinkingIdx := strings.Index(got, "Thinking:") + if contentIdx < 0 || thinkingIdx < 0 || thinkingIdx < contentIdx { + t.Errorf("Thinking must render after content: content=%d thinking=%d", contentIdx, thinkingIdx) + } + diffIdx := strings.Index(got, "+ new := 1") + if diffIdx >= 0 && thinkingIdx > diffIdx { + t.Errorf("Thinking must render before the diff block: thinking=%d diff=%d", thinkingIdx, diffIdx) + } +} + +// TestRenderComment_ThinkingEmptySkipped verifies an empty Thinking field renders +// nothing even when showThinking is true. +func TestRenderComment_ThinkingEmptySkipped(t *testing.T) { + got := captureStdout(t, func() { + renderComment(model.LlmComment{ + Path: "file.go", + StartLine: 5, + EndLine: 10, + Content: "consider renaming", + Thinking: "", + }, true) + }) + if strings.Contains(got, "Thinking:") { + t.Errorf("empty Thinking must render nothing, got:\n%s", got) + } +} + +// TestRenderComment_ThinkingSanitized verifies terminal escape sequences in +// Thinking are stripped before rendering (no raw ANSI control reaches stdout). +func TestRenderComment_ThinkingSanitized(t *testing.T) { + got := captureStdout(t, func() { + renderComment(model.LlmComment{ + Path: "file.go", + StartLine: 5, + EndLine: 10, + Content: "consider renaming", + Thinking: "look\x1b[31mred\x1b[0mhere", + }, true) + }) + if strings.Contains(got, "\x1b[31m") { + t.Errorf("raw ANSI escape in Thinking must be sanitized, got:\n%q", got) + } + if !strings.Contains(got, "look[31mred[0mhere") { + t.Errorf("sanitized Thinking should keep readable text, got:\n%q", got) + } +} diff --git a/cmd/opencodereview/retry_report_render_test.go b/cmd/opencodereview/retry_report_render_test.go index fc22a397..4e43771f 100644 --- a/cmd/opencodereview/retry_report_render_test.go +++ b/cmd/opencodereview/retry_report_render_test.go @@ -292,7 +292,7 @@ func TestRetryReport_TerminalAndJSONReadSameFrozenResult(t *testing.T) { ag := &mockResultProvider{filesReviewed: 2, manifest: mockManifest(session.StateComplete)} jsonGot := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil, nil, rep); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", false, nil, nil, rep); err != nil { t.Fatalf("emitRunResult: %v", err) } }) diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index e10844e4..f0404517 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -47,6 +47,7 @@ type reviewOptions struct { maxTokensBudget int noFilter bool preview bool + showThinking bool } var reviewOpts reviewOptions @@ -262,7 +263,7 @@ func executeReview(opts reviewOptions) error { var emitErr error emitted := manifest != nil || runErr == nil if emitted { - emitErr = emitRunResult(ctx, ag, comments, startTime, opts.outputFormat, opts.audience, q, llmIdentity, retryReport) + emitErr = emitRunResult(ctx, ag, comments, startTime, opts.outputFormat, opts.audience, opts.showThinking, q, llmIdentity, retryReport) if emitErr != nil { emitErr = fmt.Errorf("emit review result: %w", emitErr) } diff --git a/cmd/opencodereview/sarif.go b/cmd/opencodereview/sarif.go index 218a79ac..3c78e684 100644 --- a/cmd/opencodereview/sarif.go +++ b/cmd/opencodereview/sarif.go @@ -128,7 +128,7 @@ type sarifNotification struct { // warnings as tool execution notifications. When comments is empty or nil, // results is an empty array (not null), so the document remains structurally // valid for SARIF consumers. -func outputSARIF(comments []model.LlmComment, version string, warnings []agent.AgentWarning, manifest *session.RunManifest) error { +func outputSARIF(comments []model.LlmComment, version string, warnings []agent.AgentWarning, manifest *session.RunManifest, showThinking bool) error { report := sarifReport{ Schema: sarifSchema, Version: sarifVersion, @@ -141,7 +141,7 @@ func outputSARIF(comments []model.LlmComment, version string, warnings []agent.A Rules: sarifRules(), }, }, - Results: sarifResults(comments), + Results: sarifResults(comments, showThinking), Invocations: []sarifInvocation{sarifInvocationFromRun(warnings, manifest, len(comments))}, }}, } @@ -192,12 +192,12 @@ func sarifSeverityLevel(severity string) string { // SQL injection patterns in the same file), an occurrence index is appended to // each duplicate so GitHub Code Scanning tracks them as separate alerts rather // than folding them into one. -func sarifResults(comments []model.LlmComment) []sarifResult { +func sarifResults(comments []model.LlmComment, showThinking bool) []sarifResult { results := make([]sarifResult, 0, len(comments)) // Track occurrence counts per base fingerprint to disambiguate duplicates. seen := make(map[string]int, len(comments)) for _, c := range comments { - r := sarifResultFromComment(c) + r := sarifResultFromComment(c, showThinking) // Disambiguate duplicate fingerprints by appending an occurrence index. baseFP := r.PartialFingerprints[sarifFingerprintKey] count := seen[baseFP] @@ -225,16 +225,20 @@ func sarifResults(comments []model.LlmComment) []sarifResult { // the SARIF schema and cannot be omitted. When the region is invalid (zero // or inverted), the suggestion is still conveyed in message.text but no // machine-readable fix is emitted. -func sarifResultFromComment(c model.LlmComment) sarifResult { +func sarifResultFromComment(c model.LlmComment, showThinking bool) sarifResult { category := c.Category if category == "" { category = "other" } + message := c.Content + if showThinking && c.Thinking != "" { + message = c.Content + "\n\n" + "Thinking: " + c.Thinking + } result := sarifResult{ RuleID: category, Level: sarifSeverityLevel(c.Severity), - Message: sarifMessage{Text: c.Content}, + Message: sarifMessage{Text: message}, PartialFingerprints: sarifFingerprints(c, category), } diff --git a/cmd/opencodereview/sarif_test.go b/cmd/opencodereview/sarif_test.go index 21f42ee6..eca54af8 100644 --- a/cmd/opencodereview/sarif_test.go +++ b/cmd/opencodereview/sarif_test.go @@ -51,7 +51,7 @@ func TestOutputSARIF_BasicStructure(t *testing.T) { {Path: "a.go", Content: "fix", StartLine: 1, EndLine: 1, Category: "bug", Severity: "high"}, } out := captureStdout(t, func() { - if err := outputSARIF(comments, "test-version", nil, nil); err != nil { + if err := outputSARIF(comments, "test-version", nil, nil, false); err != nil { t.Fatalf("outputSARIF: %v", err) } }) @@ -92,7 +92,7 @@ func TestOutputSARIF_BasicStructure(t *testing.T) { func TestOutputSARIF_EmptyComments(t *testing.T) { out := captureStdout(t, func() { - if err := outputSARIF(nil, "test-version", nil, nil); err != nil { + if err := outputSARIF(nil, "test-version", nil, nil, false); err != nil { t.Fatalf("outputSARIF: %v", err) } }) @@ -117,7 +117,7 @@ func TestOutputSARIF_EmptyComments(t *testing.T) { func TestEmitRunResult_SarifNoFiles(t *testing.T) { ag := &mockResultProvider{filesReviewed: 0} out := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "sarif", "developer", nil, nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "sarif", "developer", false, nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -144,7 +144,7 @@ func TestOutputSARIF_FullFieldMapping(t *testing.T) { Severity: "high", } out := captureStdout(t, func() { - if err := outputSARIF([]model.LlmComment{comment}, "v1", nil, nil); err != nil { + if err := outputSARIF([]model.LlmComment{comment}, "v1", nil, nil, false); err != nil { t.Fatalf("outputSARIF: %v", err) } }) @@ -240,7 +240,7 @@ func TestOutputSARIF_EmptyCategory(t *testing.T) { Severity: "medium", } out := captureStdout(t, func() { - if err := outputSARIF([]model.LlmComment{comment}, "v1", nil, nil); err != nil { + if err := outputSARIF([]model.LlmComment{comment}, "v1", nil, nil, false); err != nil { t.Fatalf("outputSARIF: %v", err) } }) @@ -265,7 +265,7 @@ func TestOutputSARIF_ZeroLineNumbers(t *testing.T) { Severity: "high", } out := captureStdout(t, func() { - if err := outputSARIF([]model.LlmComment{comment}, "v1", nil, nil); err != nil { + if err := outputSARIF([]model.LlmComment{comment}, "v1", nil, nil, false); err != nil { t.Fatalf("outputSARIF: %v", err) } }) @@ -313,7 +313,7 @@ func TestOutputSARIF_NoFixes(t *testing.T) { Severity: "high", } out := captureStdout(t, func() { - if err := outputSARIF([]model.LlmComment{comment}, "v1", nil, nil); err != nil { + if err := outputSARIF([]model.LlmComment{comment}, "v1", nil, nil, false); err != nil { t.Fatalf("outputSARIF: %v", err) } }) @@ -381,7 +381,7 @@ func TestEmitRunResult_Sarif(t *testing.T) { {Path: "main.go", Content: "nil deref", StartLine: 10, EndLine: 10, Category: "bug", Severity: "critical"}, } out := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, comments, time.Now(), "sarif", "developer", nil, nil, nil); err != nil { + if err := emitRunResult(context.Background(), ag, comments, time.Now(), "sarif", "developer", false, nil, nil, nil); err != nil { t.Fatalf("emitRunResult: %v", err) } }) @@ -404,7 +404,7 @@ func TestOutputSARIF_SchemaCompliance(t *testing.T) { {Path: "a.go", Content: "test", StartLine: 5, EndLine: 10, Category: "security", Severity: "medium"}, } out := captureStdout(t, func() { - if err := outputSARIF(comments, "v1", nil, nil); err != nil { + if err := outputSARIF(comments, "v1", nil, nil, false); err != nil { t.Fatalf("outputSARIF: %v", err) } }) @@ -459,7 +459,7 @@ func TestOutputSARIF_SchemaCompliance(t *testing.T) { func TestOutputSARIF_JSONFormatting(t *testing.T) { out := captureStdout(t, func() { - if err := outputSARIF(nil, "v1", nil, nil); err != nil { + if err := outputSARIF(nil, "v1", nil, nil, false); err != nil { t.Fatalf("outputSARIF: %v", err) } }) @@ -481,7 +481,7 @@ func TestOutputSARIF_MultipleComments(t *testing.T) { {Path: "c.go", Content: "bad naming", StartLine: 5, EndLine: 5, Category: "style", Severity: "low"}, } out := captureStdout(t, func() { - if err := outputSARIF(comments, "v1", nil, nil); err != nil { + if err := outputSARIF(comments, "v1", nil, nil, false); err != nil { t.Fatalf("outputSARIF: %v", err) } }) @@ -523,7 +523,7 @@ func TestNewQuietHandle_Sarif(t *testing.T) { func TestSarifResultFromComment_EmptyPath(t *testing.T) { c := model.LlmComment{Content: "test", Category: "bug", Severity: "high"} - result := sarifResultFromComment(c) + result := sarifResultFromComment(c, false) if result.Locations != nil { t.Errorf("Locations should be nil when Path is empty, got %+v", result.Locations) } @@ -545,7 +545,7 @@ func TestSarifResultFromComment_InvertedLineNumbers(t *testing.T) { Category: "bug", Severity: "high", } - result := sarifResultFromComment(c) + result := sarifResultFromComment(c, false) // Locations should exist (Path is non-empty) but region must be omitted. if result.Locations == nil { @@ -576,7 +576,7 @@ func TestSarifResultFromComment_FixesWithEmptyPath(t *testing.T) { Category: "bug", Severity: "high", } - result := sarifResultFromComment(c) + result := sarifResultFromComment(c, false) if result.Locations != nil { t.Errorf("Locations should be nil when Path is empty") } @@ -723,7 +723,7 @@ func TestSarifResults_DuplicateFingerprints(t *testing.T) { {Path: "a.go", Content: "SQL injection 2", Category: "security", ExistingCode: "query('SELECT * FROM users WHERE id=' + id)", StartLine: 20, EndLine: 20}, } out := captureStdout(t, func() { - if err := outputSARIF(comments, "v1", nil, nil); err != nil { + if err := outputSARIF(comments, "v1", nil, nil, false); err != nil { t.Fatalf("outputSARIF: %v", err) } }) @@ -753,3 +753,83 @@ func TestOutputPreview_SarifRejects(t *testing.T) { t.Error("outputPreview should return an error for sarif format") } } + +// TestOutputSARIF_ThinkingHiddenByDefault verifies Thinking is NOT appended to +// message.text unless showThinking is true (backward-compatible SARIF output). +func TestOutputSARIF_ThinkingHiddenByDefault(t *testing.T) { + comment := model.LlmComment{ + Path: "a.go", + Content: "test", + Thinking: "internal reasoning", + StartLine: 1, + EndLine: 1, + Category: "bug", + Severity: "high", + } + out := captureStdout(t, func() { + if err := outputSARIF([]model.LlmComment{comment}, "v1", nil, nil, false); err != nil { + t.Fatalf("outputSARIF: %v", err) + } + }) + doc := mustUnmarshal(t, out) + result := mustGetResult(t, doc) + msg := result["message"].(map[string]any) + if msg["text"] != "test" { + t.Errorf("message.text = %v, want %q", msg["text"], "test") + } +} + +// TestOutputSARIF_ThinkingRendered verifies Thinking is appended after Content +// when showThinking is true, preserving all other fields. +func TestOutputSARIF_ThinkingRendered(t *testing.T) { + comment := model.LlmComment{ + Path: "a.go", + Content: "test", + Thinking: "internal reasoning", + StartLine: 1, + EndLine: 1, + Category: "bug", + Severity: "high", + } + out := captureStdout(t, func() { + if err := outputSARIF([]model.LlmComment{comment}, "v1", nil, nil, true); err != nil { + t.Fatalf("outputSARIF: %v", err) + } + }) + doc := mustUnmarshal(t, out) + result := mustGetResult(t, doc) + msg := result["message"].(map[string]any) + want := "test\n\n" + "Thinking: " + "internal reasoning" + if msg["text"] != want { + t.Errorf("message.text = %v, want %q", msg["text"], want) + } + // Other fields must be unchanged. + if result["ruleId"] != "bug" || result["level"] != "error" { + t.Errorf("ruleId/level changed: ruleId=%v level=%v", result["ruleId"], result["level"]) + } +} + +// TestOutputSARIF_ThinkingEmptySkipped verifies an empty Thinking field does not +// append a "Thinking: " prefix even when showThinking is true. +func TestOutputSARIF_ThinkingEmptySkipped(t *testing.T) { + comment := model.LlmComment{ + Path: "a.go", + Content: "test", + Thinking: "", + StartLine: 1, + EndLine: 1, + Category: "bug", + Severity: "high", + } + out := captureStdout(t, func() { + if err := outputSARIF([]model.LlmComment{comment}, "v1", nil, nil, true); err != nil { + t.Fatalf("outputSARIF: %v", err) + } + }) + doc := mustUnmarshal(t, out) + result := mustGetResult(t, doc) + msg := result["message"].(map[string]any) + if msg["text"] != "test" { + t.Errorf("message.text = %v, want %q", msg["text"], "test") + } +} diff --git a/cmd/opencodereview/scan_cmd.go b/cmd/opencodereview/scan_cmd.go index 94e5a3ce..5cdcf80b 100644 --- a/cmd/opencodereview/scan_cmd.go +++ b/cmd/opencodereview/scan_cmd.go @@ -36,6 +36,7 @@ type scanOptions struct { maxTools int maxGitProcs int preview bool + showThinking bool noPlan bool noDedup bool noSummary bool @@ -231,7 +232,7 @@ func executeScan(opts scanOptions) error { return fmt.Errorf("scan failed: %w", err) } - return emitRunResult(ctx, ag, comments, startTime, opts.outputFormat, opts.audience, q, llmIdentity, nil) + return emitRunResult(ctx, ag, comments, startTime, opts.outputFormat, opts.audience, opts.showThinking, q, llmIdentity, nil) } func loadScanResumeState(repoDir string, opts scanOptions, scanPaths []string) (*session.ResumeState, error) { diff --git a/cmd/opencodereview/session_cmd.go b/cmd/opencodereview/session_cmd.go index ac52d1bc..85d71700 100644 --- a/cmd/opencodereview/session_cmd.go +++ b/cmd/opencodereview/session_cmd.go @@ -198,7 +198,7 @@ func runSessionComments(sessionID string) error { return nil } for _, c := range filtered { - renderComment(c) + renderComment(c, false) } return nil } diff --git a/cmd/opencodereview/shared.go b/cmd/opencodereview/shared.go index bfa89989..e3a16493 100644 --- a/cmd/opencodereview/shared.go +++ b/cmd/opencodereview/shared.go @@ -370,6 +370,7 @@ func emitRunResult( comments []model.LlmComment, startTime time.Time, outputFormat, audience string, + showThinking bool, q *quietHandle, llmIdentity *jsonLLMIdentity, retryReport *llm.RetryReport, @@ -393,7 +394,7 @@ func emitRunResult( if outputFormat == "json" { return outputJSONNoFiles(traceID, llmIdentity) } - return outputSARIF(nil, Version, ag.Warnings(), manifest) + return outputSARIF(nil, Version, ag.Warnings(), manifest, false) } // Agent-text audiences need stdout back before PrintTraceSummary so the @@ -419,9 +420,9 @@ func emitRunResult( ag.ProjectSummary(), ag.ToolCalls(), traceID, resumeInfo, ag.SessionID(), manifest, ag.BudgetExceeded(), llmIdentity, retryReport) } if outputFormat == "sarif" { - return outputSARIF(comments, Version, ag.Warnings(), manifest) + return outputSARIF(comments, Version, ag.Warnings(), manifest, showThinking) } - outputTextWithWarnings(comments, ag.Warnings(), manifest) + outputTextWithWarnings(comments, ag.Warnings(), manifest, showThinking) // Between the comments/warnings block and the project summary: the report is // run-level diagnostics about how the comments were obtained, so it reads // after them but must not separate the summary from the end of output. diff --git a/cmd/opencodereview/shared_flags.go b/cmd/opencodereview/shared_flags.go index 6698b627..7464f701 100644 --- a/cmd/opencodereview/shared_flags.go +++ b/cmd/opencodereview/shared_flags.go @@ -65,6 +65,10 @@ func addPreviewFlag(cmd *cobra.Command, target *bool) { cmd.Flags().BoolVarP(target, "preview", "p", false, "preview which files will be reviewed without running the LLM") } +func addShowThinkingFlag(cmd *cobra.Command, target *bool) { + cmd.Flags().BoolVar(target, "show-thinking", false, "render each comment's LLM reasoning (Thinking) in terminal and SARIF output") +} + func completeEnum(values ...string) func(*cobra.Command, []string, string) ([]string, cobra.ShellCompDirective) { return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return values, cobra.ShellCompDirectiveNoFileComp @@ -180,6 +184,7 @@ func registerReviewFlags(cmd *cobra.Command, opts *reviewOptions) { addModelFlag(cmd, &opts.model) cmd.Flags().BoolVar(&opts.noFilter, "no-filter", false, "keep all review comments without LLM post-filtering") addPreviewFlag(cmd, &opts.preview) + addShowThinkingFlag(cmd, &opts.showThinking) } // registerScanFlags registers all scan command flags on cmd, binding to opts. @@ -198,6 +203,7 @@ func registerScanFlags(cmd *cobra.Command, opts *scanOptions) { cmd.Flags().IntVar(&opts.maxTokensBudget, "max-tokens-budget", 0, "cap total token usage; dispatch stops once exceeded (0 = unlimited)") cmd.Flags().StringVarP(&opts.background, "background", "b", "", "optional requirement/business context for the scan") cmd.Flags().BoolVarP(&opts.preview, "preview", "p", false, "preview which files will be scanned without running the LLM") + addShowThinkingFlag(cmd, &opts.showThinking) cmd.Flags().BoolVar(&opts.noPlan, "no-plan", false, "skip the per-file PLAN_TASK pre-pass") cmd.Flags().BoolVar(&opts.noDedup, "no-dedup", false, "skip the per-batch DEDUP_TASK") cmd.Flags().BoolVar(&opts.noSummary, "no-summary", false, "skip the post-run PROJECT_SUMMARY_TASK")