Skip to content

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

Closed
Syt3s wants to merge 1 commit into
alibaba:mainfrom
Syt3s:feat/add-show-thinking
Closed

add --show-thinking flag to render comment reasoning#889
Syt3s wants to merge 1 commit into
alibaba:mainfrom
Syt3s:feat/add-show-thinking

Conversation

@Syt3s

@Syt3s Syt3s commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Description

Adds a --show-thinking flag to ocr review and ocr scan that opt-in renders each comment's Thinking (LLM reasoning) in text and SARIF output. Default false — without the flag, output is byte-for-byte / field-for-field identical to today.

  • text: when showThinking && Thinking != "", render after the content block and before the diff, in dim style (\033[2m> Thinking: …\033[0m); Thinking is sanitized via sanitizeTerminal and wrapped via wrapByRunes(..., 100).
  • SARIF: when showThinking && Thinking != "", append c.Content + "\n\n" + "Thinking: " + c.Thinking to result.message.text; all other fields (ruleId, level, locations, fixes, partialFingerprints) unchanged.
  • ocr session is out of scope (renderComment(c, false)).

Files changed (all under cmd/opencodereview/):

File Change
shared_flags.go Add addShowThinkingFlag; register in registerReviewFlags and registerScanFlags
shared.go emitRunResult gains showThinking bool; text/sarif branches propagate it, json branch unchanged
output.go renderComment / outputText / outputTextWithWarnings render Thinking in dim style
sarif.go outputSARIF / sarifResults / sarifResultFromComment append Thinking to message.text
review_cmd.go / scan_cmd.go showThinking bool option, passed to emitRunResult
session_cmd.go renderComment(c, false)
*_test.go Call sites updated with false; new TestRenderComment_Thinking* and TestOutputSARIF_Thinking* tests

Zero new third-party Go dependencies; no changes outside cmd/opencodereview/.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • New tests: TestRenderComment_ThinkingHiddenByDefault, TestRenderComment_ThinkingRendered, TestRenderComment_ThinkingEmptySkipped, TestRenderComment_ThinkingSanitized; TestOutputSARIF_ThinkingHiddenByDefault, TestOutputSARIF_ThinkingRendered, TestOutputSARIF_ThinkingEmptySkipped (cover non-empty / empty / default combinations).
  • Regression: existing TestRenderComment*, TestOutputJSON*, TestOutputSARIF*, TestEmitRunResult* pass unchanged.
  • gofmt -s -l clean on changed files; go vet ./cmd/opencodereview/... no warnings; go build succeeds; new core functions ≥ 90% coverage.

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

Related Issues

closes #888

@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 1 issue(s) in this PR.

  • ✅ Successfully posted inline: 1 comment(s)

Comment on lines +122 to +132
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()
}

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()
}

@Syt3s
Syt3s force-pushed the feat/add-show-thinking branch from 5d11a5f to 1679c2c Compare August 13, 2026 08:35
@yingjiexu2002

Copy link
Copy Markdown
Collaborator

Thanks for the PR — the tests are solid and I like that the default behavior stays untouched.

That said, I think we'll pass on this one. Thinking is already available in JSON output since #773, so anyone who wants the reasoning can grab it there. The terminal report is meant to stay compact, and rendering the full thinking blocks would make it quite long — and since JSON already covers the use case, I'm reluctant to add a flag for it.

Thanks again for the work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: Add --show-thinking flag to render LLM reasoning in terminal and SARIF output

3 participants