Skip to content

add --show-thinking flag to render comment reasoning #889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Syt3s wants to merge 1 commit into from
Closed

add --show-thinking flag to render comment reasoning #889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 4 additions & 4 deletions cmd/opencodereview/budget_output_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
52 changes: 26 additions & 26 deletions cmd/opencodereview/emit_run_result_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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)
}
})
Expand All @@ -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)
}
})
Expand All @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
})
Expand All @@ -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)
}
})
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
})
Expand All @@ -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)
}
})
Expand All @@ -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)
}
})
Expand All @@ -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)
}
})
Expand All @@ -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)
}
})
Expand Down Expand Up @@ -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)
}
})
Expand Down
29 changes: 23 additions & 6 deletions cmd/opencodereview/output.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -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()
}
Comment on lines +122 to +133

@github-actions github-actions Bot Aug 13, 2026

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.

bug · low
The thinking content is wrapped to 100 characters, but then a prefix is prepended (> Thinking: for the first line, > for subsequent lines). This causes the actual rendered line width to exceed 100 characters (~111 chars on the first line, ~102 on subsequent lines), which is inconsistent with the content block above and can cause unwanted terminal wrapping on narrower displays.

The wrap width should account for the prefix length to maintain consistent total line width.

Suggestion:

Suggested change
if showThinking && comment.Thinking != "" {
wrapped := wrapByRunes(sanitizeTerminal(comment.Thinking), 100)
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 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 {
Expand Down
Loading