Fix trial_confirmation.go sub-step bullets to use console.FormatListItemStderr#43588
Conversation
…ItemStderr in trial_confirmation.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43588 does not have the 'implementation' label and has only 4 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
This PR updates the trial execution plan output in gh aw so the lettered sub-steps under “For each of N executions” render using the console list-item helper, making the output consistent with other styled/TTY-aware console formatting in the codebase.
Changes:
- Replaced raw
fmt.Fprintf(os.Stderr, " a./b....")lines withconsole.FormatListItemStderr(...)for the single-workflow path. - Applied the same list-item formatting change to the multi-workflow path.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/trial_confirmation.go |
Switches the “a./b.” sub-step lines to console.FormatListItemStderr for consistent, stderr-TTY-aware list formatting. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Low
| @@ -179,8 +179,8 @@ func showTrialConfirmation(opts trialConfirmationOptions) error { | |||
| workflowName := opts.parsedSpecs[0].WorkflowName | |||
| if opts.repeatCount > 0 && opts.autoMergePRs { | |||
| fmt.Fprintf(os.Stderr, console.FormatInfoMessage(" %d. For each of %d executions:\n"), stepNum, opts.repeatCount+1) | |||
| @@ -196,8 +196,8 @@ func showTrialConfirmation(opts trialConfirmationOptions) error { | |||
|
|
|||
| if opts.repeatCount > 0 && opts.autoMergePRs { | |||
| fmt.Fprintf(os.Stderr, console.FormatInfoMessage(" %d. For each of %d executions:\n"), stepNum, opts.repeatCount+1) | |||
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. The four raw fmt.Fprintf calls are cleanly replaced with console.FormatListItemStderr — the correct stderr-TTY-aware helper — matching how observability_insights.go and other callers render list items. No test regressions detected. The slight indentation change ( a. → • a.) is intentional and consistent with all surrounding steps.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 27.7 AIC · ⌖ 5.83 AIC · ⊞ 4.9K
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 36.3 AIC · ⌖ 6.79 AIC · ⊞ 6.7K
Comment /matt to run again
| @@ -179,8 +179,8 @@ func showTrialConfirmation(opts trialConfirmationOptions) error { | |||
| workflowName := opts.parsedSpecs[0].WorkflowName | |||
| if opts.repeatCount > 0 && opts.autoMergePRs { | |||
| fmt.Fprintf(os.Stderr, console.FormatInfoMessage(" %d. For each of %d executions:\n"), stepNum, opts.repeatCount+1) | |||
There was a problem hiding this comment.
[/codebase-design] The parent header on this line still uses FormatInfoMessage (stdout TTY detection) while writing to os.Stderr — the same TTY mismatch this PR fixes for the a./b. sub-steps.
💡 Suggestion
Replace FormatInfoMessage with FormatInfoMessageStderr for all fmt.Fprintf(os.Stderr, ...) call sites in this file:
// Before (still inconsistent after this PR)
fmt.Fprintf(os.Stderr, console.FormatInfoMessage(" %d. For each of %d executions:\n"), stepNum, opts.repeatCount+1)
// After (fully consistent)
fmt.Fprintf(os.Stderr, console.FormatInfoMessageStderr(" %d. For each of %d executions:\n"), stepNum, opts.repeatCount+1)There are ~15 similar call sites throughout this function. A single sweep would complete the fix the PR started.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — the fix migrates to FormatListItemStderr correctly for TTY-awareness, but introduces two visual regressions that need addressing before merge.
Blocking issues (2)
Indentation regression
FormatListItemStderr renders " • " + item — identical leading depth to the numbered parent step (" N."). The sub-steps are no longer visually subordinate; they appear as siblings. Old code used five leading spaces. See line 182–183.
Redundant bullet + letter label
The item strings "a. Execute ..." and "b. Auto-merge ..." already carry letter labels. After wrapping with FormatListItemStderr, the output is " • a. Execute ..." — both a bullet and an alphabetic label. Every other FormatListItemStderr call in the codebase passes plain unlabelled text. Pick one convention: either drop the a./b. prefixes and rely on the bullet, or use a plain fmt.Fprintf with letter labels and add TTY-aware styling manually. See line 182.
Both issues affect both the single-workflow and multi-workflow code paths identically.
🔎 Code quality review by PR Code Quality Reviewer · 57.3 AIC · ⌖ 5.33 AIC · ⊞ 5.4K
Comment /review to run again
| fmt.Fprintf(os.Stderr, " a. Execute %s\n", workflowName) | ||
| fmt.Fprintf(os.Stderr, " b. Auto-merge any pull requests created during execution\n") | ||
| fmt.Fprintln(os.Stderr, console.FormatListItemStderr(fmt.Sprintf("a. Execute %s", workflowName))) | ||
| fmt.Fprintln(os.Stderr, console.FormatListItemStderr("b. Auto-merge any pull requests created during execution")) |
There was a problem hiding this comment.
Indentation regression: the sub-steps now appear at the same column as their numbered parent, breaking the visual hierarchy.
💡 Details and suggested fix
FormatListItemStderr renders " • " + item — two leading spaces plus a bullet. The parent step line also uses two leading spaces:
3. For each of N executions:
• a. Execute workflow.md ← same column as parent!
• b. Auto-merge pull requests
The old code used five leading spaces (" a.") which placed sub-steps clearly inside the parent. The new rendering breaks the subordinate relationship.
To fix, prepend extra spaces inside the item string to push the bullet deeper:
fmt.Fprintln(os.Stderr, console.FormatListItemStderr(" a. Execute "+workflowName))
fmt.Fprintln(os.Stderr, console.FormatListItemStderr(" b. Auto-merge any pull requests created during execution"))This produces " • a. Execute ..." which indents past the parent " N.". The same fix applies to the multi-workflow branch (lines 199–200).
| fmt.Fprintf(os.Stderr, console.FormatInfoMessage(" %d. For each of %d executions:\n"), stepNum, opts.repeatCount+1) | ||
| fmt.Fprintf(os.Stderr, " a. Execute %s\n", workflowName) | ||
| fmt.Fprintf(os.Stderr, " b. Auto-merge any pull requests created during execution\n") | ||
| fmt.Fprintln(os.Stderr, console.FormatListItemStderr(fmt.Sprintf("a. Execute %s", workflowName))) |
There was a problem hiding this comment.
Redundant label + bullet: passing "a. Execute ..." into FormatListItemStderr yields " • a. Execute ..." — both a bullet point and a letter label are visible simultaneously.
💡 Details and suggested fix
FormatListItemStderr unconditionally prepends " • ". The item strings passed in are "a. Execute ..." and "b. Auto-merge ...", so the rendered lines are:
• a. Execute workflow.md
• b. Auto-merge any pull requests created during execution
Every other FormatListItemStderr call in the codebase passes plain text without letter prefixes. The original intent of these sub-steps was to use lettered labels (a/b) to indicate ordering within the parent step. A bullet prefix undermines that by adding a second, redundant visual marker.
Either drop the letter prefix and just use the bullet:
fmt.Fprintln(os.Stderr, console.FormatListItemStderr("Execute "+workflowName))
fmt.Fprintln(os.Stderr, console.FormatListItemStderr("Auto-merge any pull requests created during execution"))Or skip FormatListItemStderr and keep the letter label with appropriate indentation and TTY-aware styling directly.
|
@copilot please run the skill, address unresolved review comments, and rerun checks once the branch is up to date. Unresolved review feedback to address:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved review feedback in 936f59c. Local validation passed ( |
🤖 PR Triage — Run §28750744185
Score breakdown: Impact 12 + Urgency 8 + Quality 8 Rationale: Single-file fix migrating sub-step bullets to
Recommended next step: Defer until bot review concerns are addressed. Review in console-migration batch with #43587 and #43589.
|
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…firmation-sub-steps Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the outstanding github-actions feedback on the current head: |
|
@copilot please run the
|
No additional code change is needed on the current head: |
The two sub-step bullet pairs in
trial_confirmation.go(thea./b.lines under "For each of N executions") used rawfmt.Fprintf(os.Stderr, ...)instead of console helpers, making them visually inconsistent with all surrounding steps and stripping TTY-aware color/style in non-interactive contexts.Changes
pkg/cli/trial_confirmation.go— replaced the four rawfmt.Fprintfsub-step calls withconsole.FormatListItemStderr, covering both the single-workflow and multi-workflow code paths:FormatListItemStderrapplies TTY-aware styling (using stderr's TTY state rather than stdout's) and a•prefix, consistent with how the rest of the execution steps render.