Skip to content

Replace raw fmt.Fprintf output in outcomes_history.go with console package #39248

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

Merged
pelikhan merged 5 commits into from
Jun 14, 2026
+85 −9
Merged

Replace raw fmt.Fprintf output in outcomes_history.go with console package #39248

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fd13d94
Initial plan
Copilot Jun 14, 2026
311b16f
Replace raw fmt.Fprintf with console package calls in outcomes_histor…
Copilot Jun 14, 2026
7de4bd6
Rename ambiguous 'Label' column header to 'Bucket' in objectives hist…
Copilot Jun 14, 2026
824669f
Add unit test for pretty (non-JSON) output path in outcomes_history
Copilot Jun 14, 2026
d5298b2
Use t.Cleanup for env var teardown in TestRunOutcomesHistory_PrettyOu…
Copilot Jun 14, 2026
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
38 changes: 29 additions & 9 deletions pkg/cli/outcomes_history.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"strconv"
"strings"

"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/constants"
ghmapping "github.com/github/gh-aw/pkg/github"
"github.com/github/gh-aw/pkg/workflow"
Expand Down Expand Up @@ -162,7 +163,7 @@ func RunOutcomesHistory(config OutcomesHistoryConfig) error {
return nil
}

fmt.Fprintf(os.Stderr, "Objective history for %s (limit %d)\n", repo, config.Limit)
fmt.Fprintln(os.Stderr, console.FormatSectionHeader(fmt.Sprintf("Objective history for %s (limit %d)", repo, config.Limit)))
if data.Issues != nil {
renderHistoricalObjectiveReport(*data.Issues)
}
Expand Down Expand Up @@ -295,22 +296,41 @@ func buildHistoricalObjectiveReport(source string, items []historicalGitHubItem,
}

func renderHistoricalObjectiveReport(report historicalObjectiveReport) {
fmt.Fprintf(os.Stderr, "\n%s\n", strings.ToUpper(report.Source))
fmt.Fprintf(os.Stderr, " Sample size: %d\n", report.SampleSize)
fmt.Fprintf(os.Stderr, " Scored items: %d\n", report.ScoredItems)
fmt.Fprintf(os.Stderr, " Total objective value: %d\n", report.TotalObjectiveValue)
fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatSectionHeader(strings.ToUpper(report.Source)))

@github-actions github-actions Bot Jun 14, 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.

[/zoom-out] Minor inconsistency: the outer header (line 166) uses fmt.Fprintln, but this inner section header uses fmt.Fprintf with embedded \n characters.

The leading blank line is important for visual separation, but expressing it as a bare fmt.Fprintln(os.Stderr) followed by fmt.Fprintln(os.Stderr, console.FormatSectionHeader(...)) would be more explicit and consistent with the rest of the file.

💡 Suggested change 
// Before
fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatSectionHeader(strings.ToUpper(report.Source)))

// After
fmt.Fprintln(os.Stderr)
fmt.Fprintln(os.Stderr, console.FormatSectionHeader(strings.ToUpper(report.Source)))

This matches the fmt.Fprintln pattern used at line 166 and makes blank-line intent explicit.

@github-actions github-actions Bot Jun 14, 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.

FormatSectionHeader, FormatInfoMessage, and RenderTable all call isTTY() which resolves to tty.IsStdoutTerminal(), but every byte of output in this function is written to os.Stderr.

💡 Impact and suggested fix

This creates an inverted TTY mismatch:

  • gh aw outcomes history 2>log.txt — stdout is a terminal, isTTY() returns true, ANSI escape codes and box-drawing characters are written into log.txt.
  • gh aw outcomes history 2>&1 | less — stdout is a pipe, isTTY() returns false, all styling is stripped from what the user sees in the pager.

The console package itself demonstrates the correct pattern — RenderTitleBox/RenderErrorBox use tty.IsStderrTerminal() for their stderr output. This diff introduces a new class of stderr output using functions that check the wrong fd.

Fix options:

  1. Gate these calls: if tty.IsStderrTerminal() { fmt.Fprintln(os.Stderr, console.FormatSectionHeader(...)) } else { fmt.Fprintf(os.Stderr, "\n%s\n", ...) }
  2. Add FormatSectionHeaderForStderr / FormatInfoMessageForStderr variants that call tty.IsStderrTerminal().
  3. Pass an io.Writer + TTY boolean down into renderHistoricalObjectiveReport so callers control the decision.

fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Sample size: %d", report.SampleSize)))

@github-actions github-actions Bot Jun 14, 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.

FormatInfoMessage always emits the i Unicode character even in non-TTY mode, changing machine-parseable stderr output without warning.

💡 Details

From the console package source:

func FormatInfoMessage(message string) string {
    return applyStyle(styles.Info, "i ") + message
}

applyStyle only conditionally applies ANSI colour codes; the "i " literal prefix is always concatenated. Before this change:

  Sample size: 42
  Scored items: 10
  Total objective value: 300

After, in any non-TTY context (CI logs, 2>file, shell pipe):

i Sample size: 42
i Scored items: 10
i Total objective value: 300

Any script, grep pattern, golden-file test, or downstream parser keyed on the old format will silently break. If the intent is purely cosmetic styling, FormatInfoMessage is the wrong function for non-interactive output paths; consider only calling it when tty.IsStderrTerminal() is true, or use fmt.Fprintf(os.Stderr, " Sample size: %d\n", ...) unconditionally with styling layered on top only in TTY mode.

fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Scored items: %d", report.ScoredItems)))
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Total objective value: %d", report.TotalObjectiveValue)))

if len(report.ObjectiveBuckets) > 0 {
fmt.Fprintln(os.Stderr, " Top objective buckets:")
bucketRows := make([][]string, 0, min(len(report.ObjectiveBuckets), 8))

@github-actions github-actions Bot Jun 14, 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.

[/improve-codebase-architecture] Magic display-cap numbers (8 here and 5 at line 323) are unexplained. Named constants communicate intent and make the caps easy to adjust in one place.

💡 Suggested change

Add at the top of the file (alongside the existing const block):

const (
    maxDisplayedBuckets          = 8
    maxDisplayedRepresentativeItems = 5
)

Then use:

bucketRows := make([][]string, 0, min(len(report.ObjectiveBuckets), maxDisplayedBuckets))
for _, bucket := range report.ObjectiveBuckets[:min(len(report.ObjectiveBuckets), maxDisplayedBuckets)] {

Note: buildHistoricalObjectiveReport already caps RepresentativeItems at 15; documenting the relationship between that cap and the 5-item display limit would also clarify intent.

for _, bucket := range report.ObjectiveBuckets[:min(len(report.ObjectiveBuckets), 8)] {
fmt.Fprintf(os.Stderr, " %-22s %3d x %3d = %4d\n", bucket.Label, bucket.Count, bucket.MappedValue, bucket.ContributedValue)
bucketRows = append(bucketRows, []string{
bucket.Label,
strconv.Itoa(bucket.Count),
strconv.Itoa(bucket.MappedValue),
strconv.Itoa(bucket.ContributedValue),
})
}
fmt.Fprint(os.Stderr, console.RenderTable(console.TableConfig{

@github-actions github-actions Bot Jun 14, 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.

Bucket table loses the formula relationship between columns: the old %3d x %3d = %4d notation made count × mapped_value = contributed_value visually explicit inline; the new 4-column layout hides it.

💡 Suggested fix

The old one-liner:

label1                  3 x 10 =   30

immediately communicates that Count × MappedValue = ContributedValue. The new table rows just show three numbers and readers who are unfamiliar with the scoring model must infer the arithmetic.

Consider one of:

  1. Keep three columns but rename the header to make the formula explicit: "Count × Mapped = Contributed" as a single column showing the full expression fmt.Sprintf("%d x %d = %d", count, mapped, contributed).
  2. Drop Mapped Value and Contributed Value as separate columns and replace with a single Score (count×mapped=total) column.
  3. Add a footnote in the table title, e.g. "Top objective buckets (count × mapped value = contributed value)".

All data is still present in the table, so this is non-blocking, but the information is meaningfully less readable for diagnosing why a particular bucket dominates the objective score.

Title: "Top objective buckets",
Headers: []string{"Bucket", "Count", "Mapped Value", "Contributed Value"},
Rows: bucketRows,
}))
Comment on lines +314 to +318
}

if len(report.RepresentativeItems) > 0 {
fmt.Fprintln(os.Stderr, " Representative items:")
itemRows := make([][]string, 0, min(len(report.RepresentativeItems), 5))
for _, item := range report.RepresentativeItems[:min(len(report.RepresentativeItems), 5)] {
fmt.Fprintf(os.Stderr, " #%d %-3d %s\n", item.Number, item.ObjectiveValue, item.Title)
itemRows = append(itemRows, []string{
fmt.Sprintf("#%d", item.Number),
strconv.Itoa(item.ObjectiveValue),
item.Title,
})
}
fmt.Fprint(os.Stderr, console.RenderTable(console.TableConfig{
Title: "Representative items",
Headers: []string{"Number", "Value", "Title"},

@github-actions github-actions Bot Jun 14, 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.

[/grill-with-docs] The "Value" column header is ambiguous in the context of this table. A reader seeing the column header in isolation won't know it means "objective score". The rest of the codebase consistently uses terms like objective_value and ObjectiveValue in its domain model.

Consider "Objective" or "Score" to keep column naming aligned with the domain language and make the table self-explanatory without needing to refer to the command's broader context.

Rows: itemRows,
}))
}
}
Loading