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

[Copilot]

pkg/cli/outcomes_history.go had zero use of the console package — all user-visible output was plain fmt.Fprintf(os.Stderr, ...) with no styling.

Changes

• Section headers: title line and per-source heading now use console.FormatSectionHeader()
• Stat lines (Sample size, Scored items, Total objective value): replaced with console.FormatInfoMessage()
• Bucket table: bare formatted loop replaced with console.RenderTable() — headers: Bucket, Count, Mapped Value, Contributed Value
• Representative items: bare formatted loop replaced with console.RenderTable() — headers: Number, Value, Title
• Tests: added TestRunOutcomesHistory_PrettyOutput which captures stderr and asserts the pretty output contains the expected section headers ("Objective history for …", source names), stat lines, and both table titles/headers

// Before
fmt.Fprintf(os.Stderr, "Objective history for %s (limit %d)\n", repo, config.Limit)
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, "    %-22s %3d x %3d = %4d\n", bucket.Label, ...)

// After
fmt.Fprintln(os.Stderr, console.FormatSectionHeader(fmt.Sprintf("Objective history for %s (limit %d)", repo, config.Limit)))
fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatSectionHeader(strings.ToUpper(report.Source)))
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Sample size: %d", report.SampleSize)))
fmt.Fprint(os.Stderr, console.RenderTable(console.TableConfig{
    Title:   "Top objective buckets",
    Headers: []string{"Bucket", "Count", "Mapped Value", "Contributed Value"},
    Rows:    bucketRows,
}))

[Copilot]

Pull request overview

This PR updates the outcomes history CLI command’s human-readable output to use the shared pkg/console formatting utilities (styled section headers/info messages and rendered tables) instead of raw fmt.Fprintf output, aligning the command with other CLI output patterns in the codebase.

Changes:

• Replace plain text section headers with console.FormatSectionHeader(...).
• Replace stat lines with console.FormatInfoMessage(...).
• Render objective bucket and representative item lists using console.RenderTable(...).

Show a summary per file

File	Description
pkg/cli/outcomes_history.go	Switches user-visible (stderr) output to console-formatted headers/info messages and table rendering for buckets/items.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 1/1 changed files
• Comments generated: 1

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. Only pkg/cli/outcomes_history.go was changed (production code only). Test Quality Sentinel skipped.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #39248 does not have the implementation label (has_implementation_label=false) and has only 29 new lines in business logic directories (default_business_additions=29, threshold=100; requires_adr_by_default_volume=false). Neither Condition A nor Condition B is met.

[github-actions]

Skills-Based Review 🧠

Applied /zoom-out, /improve-codebase-architecture, and /grill-with-docs — approving with minor suggestions.

📋 Key Themes & Highlights

Key Themes

• Fmt.Fprintf inconsistency (line 299): inner section headers use fmt.Fprintf with embedded \n; outer header uses fmt.Fprintln. Minor, but easy to align.
• Magic numbers (lines 305, 323): display caps of 8 and 5 should be named constants for clarity and discoverability.
• Ambiguous column header (line 332): "Value" in the representative items table could be "Objective" or "Score" to match domain vocabulary.

Positive Highlights

• ✅ Correct FormatInfoMessage call pattern: using fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf(...))) is the safe approach — this is better than the pre-existing pattern in trial_confirmation.go where the styled string is passed as a format string to fmt.Fprintf, which is a latent bug.
• ✅ Clean table conversion: the two ad-hoc loops are now proper RenderTable calls with explicit column headers — significant readability improvement.
• ✅ Well-scoped: single file, +29/-9, zero logic change.

Test Coverage Note

renderHistoricalObjectiveReport has no test coverage. Since output always goes to os.Stderr this requires a bit of effort (e.g. capture via os.Pipe as done in TestRunOutcomesHistory_JSON), but a snapshot test would give confidence that future console API changes don't silently break the output structure.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 375.3 AIC · ⌖ 13.7 AIC · ⊞ 27.9K

[github-actions]

[/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]

[/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.

[github-actions]

[/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.

[github-actions]

Review: outcomes_history.go console styling migration

The change is clean and correctly migrates all output through the console package. One non-blocking concern about information density in the bucket table.

Details

What was verified

• RenderTable returns a trailing \n; fmt.Fprint (not fmt.Fprintln) is correctly used — no double-newline.
• FormatSectionHeader does not add a trailing newline; fmt.Fprintln on line 166 is correct.
• strconv was already imported; new console import is the only addition.
• All min(len(...), N) slice patterns match the original and are safe.
• RenderTable with an empty Headers slice returns "" — but headers are hardcoded strings here, so this guard never triggers.

Concern

• See inline comment on line 314 — the bucket table drops the inline formula notation (count × mapped = contributed) that made the scoring breakdown self-explanatory.

Sub-agent note

The grumpy-coder sub-agent did not complete within the timeout window; findings above are from independent second-pass analysis.

🔎 Code quality review by PR Code Quality Reviewer · 374.2 AIC · ⌖ 13.3 AIC · ⊞ 17.3K

[github-actions]

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.

[github-actions]

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.

[github-actions]

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.

[github-actions]

Hey @copilot-swe-agent 👋 — nice work migrating outcomes_history.go to the console package! The switch from raw fmt.Fprintf(os.Stderr, ...) to console.FormatSectionHeader(), console.FormatInfoMessage(), and console.RenderTable() is exactly the kind of consistency improvement this codebase benefits from.

One thing that would strengthen this PR:

• Add tests — renderHistoricalObjectiveReport is now non-trivially producing structured table output via console.RenderTable(), but there's no test coverage for the new rendering logic. A test that exercises renderHistoricalObjectiveReport (or its callers via RunOutcomesHistory) and asserts on the formatted output would ensure the console rendering behaves as expected and doesn't regress.

If you'd like a hand, you can assign this prompt to your coding agent:

Add tests for the renderHistoricalObjectiveReport function in pkg/cli/outcomes_history.go.

The function was recently refactored to use console.FormatSectionHeader(), console.FormatInfoMessage(), and console.RenderTable() instead of raw fmt.Fprintf calls. Write unit tests that:
1. Construct a sample historicalObjectiveReport with a populated Source, SampleSize, ScoredItems, TotalObjectiveValue, ObjectiveBuckets (at least 2 entries), and RepresentativeItems (at least 2 entries).
2. Capture stderr output (redirect os.Stderr or use a buffer via the console package's test helpers if available).
3. Assert that the output contains the section header, the stat lines, and the expected table headers ("Bucket", "Count", "Mapped Value", "Contributed Value", "Number", "Value", "Title").
4. Test the edge case where ObjectiveBuckets and RepresentativeItems are both empty — output should only contain the header and stat lines.

Place the tests in pkg/cli/outcomes_history_test.go.

Generated by ✅ Contribution Check · 503.3 AIC · ⌖ 13.6 AIC · ⊞ 24.7K · ◷

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Added TestRunOutcomesHistory_PrettyOutput in the latest commits — it uses testutil.CaptureStderr to capture stderr and asserts the non-JSON output contains the expected section headers ("Objective history for owner/repo (limit 10)", "ISSUES", "PRS"), stat lines ("Sample size", "Scored items", "Total objective value"), and both table titles/headers ("Top objective buckets" with "Bucket", "Mapped Value", "Contributed Value", and "Representative items" with "Number", "Value", "Title").

[github-actions]

@copilot review all comments and address unresolved review feedback.
Please add a stderr capture test and resolve the open review threads.

Generated by 👨‍🍳 PR Sous Chef · 63.3 AIC · ⌖ 1.03 AIC · ⊞ 17.4K · ◷