Wrap org runner and MCP list-tools stderr output with console formatting helpers#43589
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43589 does not have the 'implementation' label and has 87 new lines of code in business logic directories (≤100 threshold). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR standardizes a couple of CLI stderr output paths to go through the console formatting layer, so messages written to stderr get consistent prefixes and correct TTY-aware styling/stripping.
Changes:
- Updated org runner summary lines to use
console.FormatListItemStderr(...)instead of manual bullet formatting. - Updated MCP
list-toolsstatus messages to useconsole.FormatInfoMessageStderr(...)and added clearer blank-line separation for headings. - Added unit tests to pin the intended stderr formatting behavior for the affected output paths.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/stderr_console_formatting_test.go | Adds focused unit tests asserting stderr output is routed through stderr-aware console formatters. |
| pkg/cli/org_runner.go | Routes org action summary “list item” lines through FormatListItemStderr instead of raw stderr bullets. |
| pkg/cli/mcp_list_tools.go | Routes MCP list-tools discovery/status output through FormatInfoMessageStderr and adjusts headings/spacing. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Low
| func renderOrgActionSummary(preview orgRepoPreview, action string) { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Ready to "+action)) | ||
| fmt.Fprintf(os.Stderr, " - Repository: %s\n", preview.Repo) | ||
| fmt.Fprintln(os.Stderr, console.FormatListItemStderr("Repository: "+preview.Repo)) |
| // Connect to the MCP server and get its tools | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("📡 Connecting to MCP server: %s (%s)", | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessageStderr(fmt.Sprintf("📡 Connecting to MCP server: %s (%s)", | ||
| targetConfig.Name, | ||
| targetConfig.Type))) |
🧪 Test Quality Sentinel Report
📊 Metrics (2 tests)
|
Skills-Based Review 🧠Applied 📋 Key Themes & HighlightsKey Themes
Positive Highlights
@copilot please address the review comments above.
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 55.2 AIC · ⌖ 6.85 AIC · ⊞ 6.7K
Comment /matt to run again
| @@ -84,23 +84,23 @@ type orgRunCallbacks struct { | |||
|
|
|||
| func renderOrgActionSummary(preview orgRepoPreview, action string) { | |||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Ready to "+action)) | |||
There was a problem hiding this comment.
[/codebase-design] This line writes to os.Stderr but calls FormatInfoMessage (stdout TTY check), which is exactly the inconsistency this PR aims to fix — it should use FormatInfoMessageStderr.
💡 Suggested fix
fmt.Fprintln(os.Stderr, console.FormatInfoMessageStderr("Ready to "+action))FormatInfoMessage calls isTTY (stdout), while FormatInfoMessageStderr calls isStderrTTY. When stdout is redirected but stderr is a terminal (e.g. piped commands), this line will lose its ANSI styling while the surrounding bullet lines render correctly — exactly the TTY-safe inconsistency the PR description says it resolves.
@copilot please address this.
|
|
||
| lines := strings.Split(strings.TrimSuffix(stderr, "\n"), "\n") | ||
| require.Len(t, lines, 5) | ||
| assert.Equal(t, console.FormatInfoMessage("Ready to update"), lines[0]) |
There was a problem hiding this comment.
[/tdd] The test asserts console.FormatInfoMessage for lines[0], but that's the stdout-TTY variant. If the production fix on line 86 of org_runner.go is made (using FormatInfoMessageStderr), this assertion will need to change too — and the test would currently pass for the wrong reason.
💡 Suggested fix
assert.Equal(t, console.FormatInfoMessageStderr("Ready to update"), lines[0])Tests that encode the wrong helper as the expected value become false-positive greenlight for the bug they were meant to catch. The test name says "UsesConsoleFormatListItemStderr" but the heading assertion silently accepts a non-stderr formatter.
@copilot please address this.
There was a problem hiding this comment.
Review: Wrap org runner and MCP list-tools stderr output with console formatting helpers
Good direction — routing raw fmt.Fprintf calls through the console formatting layer is the right approach. Three issues need attention before this lands:
Blocking:
-
Flat hierarchy for nested workflow items (line 94,
org_runner.go): Sub-items under "Pending workflow updates:" now render at the same•indent level as the parent, erasing the previous 4-space indent that distinguished them. The formatted output regresses in readability for workflows with pending updates. -
FormatInfoMessage(stdout TTY) on a stderr write (line 86,org_runner.go): The PR migrated all surrounding lines toStderr-variants but left the "Ready to (action)" header using the stdout-TTYFormatInfoMessage. In a non-TTY stdout / TTY stderr context, the prefix formatting will be wrong for that one line.
Non-blocking:
- Test asserts the wrong variant (line 34, test file): The new test anchors the header on
FormatInfoMessage, which cements the inconsistency rather than verifying the intended stderr-safe path.
See inline comments for specific locations.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 60.9 AIC · ⌖ 5.25 AIC · ⊞ 4.9K
| fmt.Fprintln(os.Stderr, console.FormatListItemStderr("Pending workflow updates:")) | ||
| for _, wf := range preview.Workflows { | ||
| fmt.Fprintf(os.Stderr, " - %s: %s -> %s\n", wf.Name, wf.CurrentRef, wf.LatestRef) | ||
| fmt.Fprintln(os.Stderr, console.FormatListItemStderr(fmt.Sprintf("%s: %s -> %s", wf.Name, wf.CurrentRef, wf.LatestRef))) |
There was a problem hiding this comment.
The sub-items for each workflow entry (e.g. repo-assist: v1.0.0 -> v1.1.0) now use the same FormatListItemStderr level as the parent "Pending workflow updates:" heading. Previously these were rendered with 4-space indent ( - name: old -> new), providing clear visual nesting under their parent.
After this change the output is flat:
• Pending workflow updates:
• repo-assist: v1.0.0 -> v1.1.0
Consider adding a FormatSubListItemStderr helper (or similar) to preserve the two-level hierarchy, or explicitly document that this flattening is an intentional design simplification.
@copilot please address this.
| @@ -84,23 +84,23 @@ type orgRunCallbacks struct { | |||
|
|
|||
| func renderOrgActionSummary(preview orgRepoPreview, action string) { | |||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Ready to "+action)) | |||
There was a problem hiding this comment.
FormatInfoMessage uses the stdout TTY check (isTTY) while the output is going to os.Stderr. All surrounding lines (87–103) correctly use the Stderr variants. The "Ready to (action)" header line should use FormatInfoMessageStderr for consistency:
fmt.Fprintln(os.Stderr, console.FormatInfoMessageStderr("Ready to "+action))This is a pre-existing inconsistency that still remains on this line — the PR fixed the sibling calls but missed this one.
@copilot please address this.
|
|
||
| lines := strings.Split(strings.TrimSuffix(stderr, "\n"), "\n") | ||
| require.Len(t, lines, 5) | ||
| assert.Equal(t, console.FormatInfoMessage("Ready to update"), lines[0]) |
There was a problem hiding this comment.
Line 34 asserts the header uses FormatInfoMessage (stdout-TTY variant), which matches the current code but perpetuates the inconsistency. If the intent is to migrate all renderOrgActionSummary output to the stderr-aware helpers, this assertion should verify FormatInfoMessageStderr instead. As-is the test encodes the inconsistency rather than driving the correct behaviour.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness issues, one maintainability issue
The console-layer migration is the right approach, but there are two bugs that must be fixed before merging.
Blocking issues (2 correctness)
1. org_runner.go:86 — FormatInfoMessage on a stderr write
The header line Ready to <action> still uses FormatInfoMessage, which checks isTTY() (stdout TTY), while every list item beneath it uses FormatListItemStderr (stderr TTY). In any piped invocation where stdout is redirected but stderr is not, the header will render without color/prefix while its children are styled—a visually broken block. Fix: console.FormatInfoMessageStderr("Ready to "+action).
2. stderr_console_formatting_test.go:34 — test asserts the wrong variant
The test asserting line 0 uses console.FormatInfoMessage, locking in the bug instead of catching it. Fix alongside the production code change.
Non-blocking (1 maintainability)
mcp_list_tools.go:136, 151 — raw fmt.Fprintln(os.Stderr, "") for blank lines
Two blank-line separators bypass the console layer entirely. Not a runtime bug, but breaks the abstraction the PR is establishing. Consider a console.PrintBlankLineStderr() helper or at least a comment noting the intentional bypass.
🔎 Code quality review by PR Code Quality Reviewer · 221.1 AIC · ⌖ 9.22 AIC · ⊞ 5.4K
Comment /review to run again
| @@ -84,23 +84,23 @@ type orgRunCallbacks struct { | |||
|
|
|||
| func renderOrgActionSummary(preview orgRepoPreview, action string) { | |||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Ready to "+action)) | |||
There was a problem hiding this comment.
Wrong TTY check on the Ready to header line: FormatInfoMessage checks stdout TTY while the line is written to stderr; all child list items correctly use FormatListItemStderr (stderr TTY), so this header will lose color/styling whenever stdout is not a TTY but stderr is—producing an inconsistently formatted block.
💡 Suggested fix
Change line 86 in org_runner.go to use the stderr-aware variant:
fmt.Fprintln(os.Stderr, console.FormatInfoMessageStderr("Ready to "+action))The surrounding list items already use FormatListItemStderr, so this header must match. FormatInfoMessage routes the TTY check through isTTY() (stdout), which can disagree with isStderrTTY() (stderr) in piped/redirected invocations.
|
|
||
| lines := strings.Split(strings.TrimSuffix(stderr, "\n"), "\n") | ||
| require.Len(t, lines, 5) | ||
| assert.Equal(t, console.FormatInfoMessage("Ready to update"), lines[0]) |
There was a problem hiding this comment.
Test asserts the wrong format function, hardcoding a bug: Line 34 asserts console.FormatInfoMessage("Ready to update") (stdout-TTY-aware), but the production code at org_runner.go:86 actually calls console.FormatInfoMessage too—both agree, but both are wrong (should be FormatInfoMessageStderr). This test cements the inconsistency rather than catching it; it will pass even after the TTY-check bug is introduced or persists.
💡 Suggested fix
Fix the assertion to expect the stderr-aware variant (and fix the production code on org_runner.go:86 to match):
assert.Equal(t, console.FormatInfoMessageStderr("Ready to update"), lines[0])Once org_runner.go:86 is also updated to FormatInfoMessageStderr, this test will correctly gate against future regressions where someone re-introduces the stdout-TTY variant.
| fmt.Fprintln(os.Stderr, console.FormatInfoMessageStderr( | ||
| fmt.Sprintf("Found MCP server '%s' in %d workflow(s): %s", mcpServerName, len(matchingWorkflows), strings.Join(matchingWorkflows, ", ")), | ||
| )) | ||
| fmt.Fprintln(os.Stderr, "") |
There was a problem hiding this comment.
Raw blank-line emission bypasses the console abstraction: fmt.Fprintln(os.Stderr, "") is a naked call that bypasses the console formatting layer the rest of this PR is trying to enforce. It mixes raw stderr writes with console-wrapped writes in the same output stream.
💡 Suggested fix
While this is a cosmetic emission, the inconsistency matters for consumers (e.g. log capturers, test helpers) that strip or transform console-prefixed lines. If the console package has a blank-line helper, use it; otherwise at minimum document the intentional bypass. For example:
// Blank separator: intentionally raw (no console prefix)
_, _ = fmt.Fprintln(os.Stderr)Or, if the same pattern is needed in multiple places, add a console.PrintBlankLineStderr() helper to keep all stderr I/O routed through one layer. The same pattern appears at line 151 in displayToolsList.
|
@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>
Fixed in the latest commit. All three
|
This updates two CLI paths that were still writing user-facing stderr output directly, bypassing the console formatting layer. The change routes those messages through the existing stderr-aware console helpers so org summaries and MCP tool-list status output use the standard prefixes and TTY-safe formatting.
Org runner summary output
pkg/cli/org_runner.gowithconsole.FormatListItemStderr(...).MCP list-tools status output
pkg/cli/mcp_list_tools.gowithconsole.FormatInfoMessageStderr(...).Regression coverage
Example of the change pattern: