Add console.Print* stderr wrappers and migrate high-volume MCP/secrets callsites#47108
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
console.Print* stderr wrappers and migrate high-volume MCP/secrets callsites
This comment has been minimized.
This comment has been minimized.
🤖 PR Triage
Score breakdown: Impact 22 · Urgency 12 · Quality 11
|
|
This PR looks great! 🎉 The refactor consolidates stderr output ergonomics in the Strengths:
This is ready for review by core team members!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds stderr-oriented console printing helpers and migrates high-volume CLI callsites.
Changes:
- Adds and tests
console.Print*wrappers. - Migrates MCP inspection and secret-management output.
- Updates usage documentation and a generated workflow permission.
Show a summary per file
| File | Description |
|---|---|
pkg/console/print.go |
Adds stderr print wrappers. |
pkg/console/print_test.go |
Tests wrapper output. |
pkg/console/README.md |
Documents direct print helpers. |
pkg/console/doc.go |
Updates package guidance. |
pkg/cli/mcp_inspect_mcp.go |
Migrates MCP diagnostics. |
pkg/cli/engine_secrets.go |
Migrates secret-management diagnostics. |
.github/workflows/smoke-call-workflow.lock.yml |
Reduces an unrelated generated permission. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 5
- Review effort level: Medium
| func PrintSuccessMessage(message string) (int, error) { | ||
| return fmt.Fprintln(os.Stderr, FormatSuccessMessage(message)) |
| func PrintErrorChain(err error) (int, error) { | ||
| return fmt.Fprintln(os.Stderr, FormatErrorChain(err)) |
| return string(out) | ||
| } | ||
|
|
||
| func TestPrintMessageHelpers(t *testing.T) { |
| # Review the called workflow's job-level permissions in ./.github/workflows/smoke-workflow-call.lock.yml. | ||
| permissions: | ||
| actions: write | ||
| actions: read |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (2 tests)
📝 Notes
Verdict
|
There was a problem hiding this comment.
Review: console.Print* stderr wrappers
Good consolidation overall — the intent of the PR is sound and the migration in engine_secrets.go and mcp_inspect_mcp.go is clean and consistent.
One blocking correctness issue was found in pkg/console/print.go:
Four Print* wrappers call the stdout-oriented Format* functions instead of their *Stderr counterparts:
| Wrapper | Currently calls | Should call |
|---|---|---|
PrintSuccessMessage |
FormatSuccessMessage (uses isTTY + applyStdoutStyleWithTTY) |
FormatSuccessMessageStderr |
PrintInfoMessage |
FormatInfoMessage (uses isTTY + applyStdoutStyleWithTTY) |
FormatInfoMessageStderr |
PrintSectionHeader |
FormatSectionHeader (uses isTTY + applyStdoutStyleWithTTY) |
FormatSectionHeaderStderr |
PrintListItem |
FormatListItem (uses isTTY + applyStdoutStyleWithTTY) |
FormatListItemStderr |
The stdout variants use isTTY (is stdout a terminal?) and apply colour-profile degradation via colorwriter.Degrade, which is only appropriate for stdout. When these wrappers write to os.Stderr, the styling decision is based on the wrong file descriptor — producing stray ANSI codes when stderr is redirected but stdout is a TTY, or plain output when stderr is a colour terminal but stdout is not.
The test expectations in print_test.go also need updating to reference the *Stderr format functions once print.go is fixed (inline comment posted there too).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 40 AIC · ⌖ 5.42 AIC · ⊞ 5K
| // PrintInfoMessage formats and prints an info message to stderr. | ||
| func PrintInfoMessage(message string) (int, error) { | ||
| return fmt.Fprintln(os.Stderr, FormatInfoMessage(message)) | ||
| } |
There was a problem hiding this comment.
Wrong TTY check: use FormatSuccessMessageStderr instead of FormatSuccessMessage
PrintSuccessMessage writes to os.Stderr, but it calls FormatSuccessMessage which uses isTTY (stdout TTY check) and applyStdoutStyleWithTTY — correct only for stdout. For stderr output, call FormatSuccessMessageStderr which uses isStderrTTY and applyStyleWithTTY.
The same mistake affects three other wrappers:
PrintInfoMessage→ should callFormatInfoMessageStderrPrintSectionHeader→ should callFormatSectionHeaderStderrPrintListItem→ should callFormatListItemStderr
When stdout is a TTY but stderr is not (or vice-versa), styling will be applied or stripped incorrectly, producing ANSI escape codes in redirected stderr or plain text when stderr is a colour terminal.
@copilot please address this.
| if stderr != want { | ||
| t.Fatalf("expected %q, got %q", want, stderr) | ||
| } | ||
| } |
There was a problem hiding this comment.
Tests validate against stdout Format* functions but Print* writes to stderr
The test assertions compare output against FormatSuccessMessage("ok"), FormatInfoMessage("info"), etc. — which use the stdout TTY check. Once the wrappers are corrected to call the *Stderr variants (see sibling comment on print.go), these comparisons will pass incidentally only when stdout and stderr have the same TTY status in the test environment, but they do not validate the actual contract.
After fixing print.go, update the expected strings to call FormatSuccessMessageStderr, FormatInfoMessageStderr, FormatSectionHeaderStderr, and FormatListItemStderr to match what the wrappers actually produce.
@copilot please address this.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (248 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on interface design and test completeness.
📋 Key Themes & Highlights
Key Themes
- Interface noise: All 13
Print*wrappers return(int, error), but every call site discards those values with_, _ =. Void returns would be cleaner and more honest (see inline onprint.go:14). - Incomplete test coverage: 7 of the 13 new wrappers have no test. The batched
t.Fatalfstyle also hides failures past the first (see inline onprint_test.go:40). - Test parallelism hazard:
captureStderrmutates the globalos.Stderr; parallel tests in the same package would race (see inline onprint_test.go:22). - README example: code snippet omits
_, _ =discards, making it invalid Go unless the signature is changed to void (see inline onREADME.md).
Positive Highlights
- ✅ Clean, consistent wrapper layer — the abstraction is exactly the right shape.
- ✅ Migration in
engine_secrets.goandmcp_inspect_mcp.gois thorough and mechanical — great reduction in boilerplate. - ✅ Correct handling of the
\n%s\n→Fprintln + Print*decomposition indisplayServerCapabilities, preserving blank-line spacing.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 45.7 AIC · ⌖ 5.51 AIC · ⊞ 6.7K
Comment /matt to run again
| } | ||
|
|
||
| // PrintSuccessMessage formats and prints a success message to stderr. | ||
| func PrintSuccessMessage(message string) (int, error) { |
There was a problem hiding this comment.
[/codebase-design] All 13 Print* wrappers return (int, error), but every call site immediately discards them with _, _ =. If the return values are never meaningful to callers, making these functions return nothing (func PrintSuccessMessage(message string)) removes the noise and signals the intent more clearly — stderr writes failing silently is already an accepted trade-off at this layer.
💡 Suggested signature
func PrintSuccessMessage(message string) {
fmt.Fprintln(os.Stderr, FormatSuccessMessage(message))
}If surfacing the error to callers is ever needed in future, it's easy to add. Starting with a void return is a simpler, more honest interface.
@copilot please address this.
| return string(out) | ||
| } | ||
|
|
||
| func TestPrintMessageHelpers(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] TestPrintMessageHelpers batches 6 assertions in one test with t.Fatalf — the first failure hides all subsequent ones. Split into sub-tests (or use t.Errorf) so you can see all failures at once.
💡 Suggested pattern
cases := []struct{
name string
fn func() (int, error)
want string
}{
{"success", func() (int, error) { return PrintSuccessMessage("ok") }, FormatSuccessMessage("ok")},
// ...
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
got := captureStderr(t, func() { tc.fn() })
if !strings.Contains(got, tc.want+"\n") {
t.Errorf("want %q in stderr, got %q", tc.want, got)
}
})
}@copilot please address this.
| return string(out) | ||
| } | ||
|
|
||
| func TestPrintMessageHelpers(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The test covers 6 of the 13 new Print* functions — PrintProgressMessage, PrintPromptMessage, PrintVerboseMessage, PrintListItem, PrintTableHeaderStderr, PrintErrorTextStderr, and PrintError have no test coverage. Add them to the table-driven test so regressions on any wrapper are caught.
@copilot please address this.
| console.PrintInfoMessage("Processing 3 files...") | ||
| console.PrintWarningMessage("Network access is unrestricted") | ||
| console.PrintErrorMessage("File not found: workflow.md") | ||
| console.PrintCommandMessage("gh aw compile workflow.md") |
There was a problem hiding this comment.
[/codebase-design] The README example drops the return-value discards (_, _ =) that every real call site uses. Either document the canonical call pattern including _, _ = (or a void form if the signature is simplified), so readers copy-paste a valid Go snippet.
@copilot please address this.
| if err != nil { | ||
| t.Fatalf("failed to create stderr pipe: %v", err) | ||
| } | ||
| os.Stderr = w |
There was a problem hiding this comment.
[/tdd] captureStderr reassigns the global os.Stderr. If tests run with t.Parallel() in the same package later, they will race on the global. Consider injecting an io.Writer into Print* functions (or at least document that these tests cannot be parallelised).
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — Three issues need fixing before merge
The API design and test quality need corrections.
🔍 Key findings
High: Misleading (int, error) return signature
Every call site uses _, _ = console.Print*(), silently discarding both return values. The API implies callers should check errors but no one does. The functions should return nothing, which also eliminates the _, _ = noise throughout the codebase (~30 call sites).
High: captureStderr is a data race
os.Stderr is a package-level variable mutated without synchronization. Running tests with -race or in parallel will flag this. The root fix is to make Print* accept an io.Writer or use a package-level var stderr io.Writer = os.Stderr.
Medium: strings.Contains test assertions are too weak
TestPrintMessageHelpers batches all calls and checks the combined output with Contains. A copy-paste bug assigning the wrong Format* call to a wrapper would go undetected. Each wrapper should be tested in isolation with exact equality.
Medium: 7 of 13 new wrappers are untested
PrintTableHeaderStderr, PrintErrorTextStderr, PrintProgressMessage, PrintPromptMessage, PrintVerboseMessage, PrintListItem, and PrintError have no test coverage.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 57.1 AIC · ⌖ 5.03 AIC · ⊞ 5.6K
Comment /review to run again
| } | ||
|
|
||
| // PrintSuccessMessage formats and prints a success message to stderr. | ||
| func PrintSuccessMessage(message string) (int, error) { |
There was a problem hiding this comment.
(int, error) return values are universally discarded — the API contract is misleading.
Every single call site uses _, _ = console.Print*(). If write errors are intentionally ignored, these functions should return nothing.
💡 Suggested fix
Change all Print* signatures to void:
// before
func PrintSuccessMessage(message string) (int, error) {
return fmt.Fprintln(os.Stderr, FormatSuccessMessage(message))
}
// after
func PrintSuccessMessage(message string) {
fmt.Fprintln(os.Stderr, FormatSuccessMessage(message)) (nolint/redacted):errcheck
}This also eliminates the _, _ = noise at every call site.
| func TestPrintMessageHelpers(t *testing.T) { | ||
| stderr := captureStderr(t, func() { | ||
| _, _ = PrintSuccessMessage("ok") | ||
| _, _ = PrintInfoMessage("info") |
There was a problem hiding this comment.
Data race: captureStderr mutates the global os.Stderr without synchronization.
If tests run in parallel (or background goroutines write to stderr), this is a race condition that -race will flag.
💡 Suggested fix
Instead of swapping os.Stderr, accept an io.Writer in Print* functions (or use a package-level var stderr io.Writer) so tests can pass bytes.Buffer directly:
var stderr io.Writer = os.Stderr
func PrintSuccessMessage(message string) {
fmt.Fprintln(stderr, FormatSuccessMessage(message))
}Then in tests:
var buf bytes.Buffer
stderr = &buf
defer func() { stderr = os.Stderr }()
PrintSuccessMessage("ok")This avoids touching OS-level file descriptors entirely.
| func TestPrintErrorChain(t *testing.T) { | ||
| err := errors.New("inner") | ||
| wrapped := fmt.Errorf("outer: %w", err) | ||
|
|
There was a problem hiding this comment.
strings.Contains assertions cannot detect ordering errors, duplicate output, or wrong-function attribution.
If PrintSuccessMessage accidentally printed FormatInfoMessage's output (due to a copy-paste bug), this test would still pass because both strings are present somewhere in the combined capture.
💡 Suggested fix
Capture each Print* call individually and assert exact equality:
cases := []struct {
fn func()
want string
}{
{func() { PrintSuccessMessage("ok") }, FormatSuccessMessage("ok") + "\n"},
{func() { PrintInfoMessage("info") }, FormatInfoMessage("info") + "\n"},
// ...
}
for _, tc := range cases {
got := captureStderr(t, tc.fn)
if got != tc.want {
t.Errorf("got %q, want %q", got, tc.want)
}
}| if stderr != want { | ||
| t.Fatalf("expected %q, got %q", want, stderr) | ||
| } | ||
| } |
There was a problem hiding this comment.
7 of 13 new Print* wrappers have zero test coverage.
PrintTableHeaderStderr, PrintErrorTextStderr, PrintProgressMessage, PrintPromptMessage, PrintVerboseMessage, PrintListItem, and PrintError are not tested at all. A silent regression in any underlying Format* function (e.g., wrong argument passing) would go undetected.
💡 Suggested fix
Add table-driven test cases for each missing wrapper, following the same pattern as the existing tests. Since these wrappers are trivial one-liners, a single table-driven test covering all 13 is straightforward.
- Fix PrintError to use Fprint (FormatError already ends with \n) - Add *Stderr-aware Format variants for Warning/Command/Progress/Prompt/Verbose - Update all Print* wrappers to use *Stderr variants for correct styling - Add FormatErrorChain and all missing *Stderr stubs to console_wasm.go - Expand print_test.go to cover all 14 wrappers with exact newline assertions" Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all five review threads:
|
…tderr variable - Change all Print* signatures from (int, error) to void — write errors on stderr are intentionally not surfaced at this diagnostic layer - Add var stderr io.Writer = os.Stderr so tests can capture output via a bytes.Buffer without touching OS file descriptors - Remove _, _ = prefix from all call sites in engine_secrets.go and mcp_inspect_mcp.go - Update print_test.go to use the package-level stderr variable via captureStderr, removing the OS pipe approach and documenting the no-parallel constraint Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all remaining review threads:
Commit: refactor(console): make Print* functions void and use package-level stderr variable |
|
🎉 This pull request is included in a new release. Release: |
pkg/consoleexposedFormat*helpers but no direct stderr print API, which led to widespread two-stepfmt.Fprintln(os.Stderr, console.Format*(...))patterns and inconsistent output routing ergonomics. This change introduces thinPrint*wrappers and migrates the two largest offenders in CLI code.Console output API consolidation
pkg/console/print.gothat mirror existingFormat*helpers and centralize newline + destination handling (os.Stderr).Targeted migration in highest-volume files
pkg/cli/mcp_inspect_mcp.gopkg/cli/engine_secrets.goFprintln(..., console.Format*)and newline-wrappedFprintf("\n%s\n", console.Format*)patterns toconsole.Print*while preserving message structure.Docs + usage guidance alignment
pkg/console/doc.go,pkg/console/README.md) to preferPrint*for diagnostic stderr output.Focused wrapper coverage
pkg/console/print_test.goto assert that newPrint*wrappers emit expected formatted output to stderr.Run: https://github.com/github/gh-aw/actions/runs/29895667664
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
run: https://github.com/github/gh-aw/actions/runs/29898890921
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.