Route human-readable messages to stderr when --output json is active#7393
Route human-readable messages to stderr when --output json is active#7393
Conversation
spboyer
requested review from
JeffreyCA,
hemarina,
jongio,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
There was a problem hiding this comment.
Pull request overview
This PR updates the input console implementation so that when --output json is active, human-oriented console messages (including spinner fallback text) are written to stderr, keeping stdout clean for machine-readable JSON output.
Changes:
- Added a JSON-output mode flag to the console and centralized message routing via a
messageWriter()helper. - Updated
Message,MessageUxItem, and spinner-related methods to use stderr in JSON mode. - Expanded/updated unit tests to validate stderr routing in JSON mode and preserve behavior in non-JSON modes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cli/azd/pkg/input/console.go |
Introduces JSON-mode-aware message routing and applies it across message/spinner output paths. |
cli/azd/pkg/input/console_test.go |
Updates existing JSON tests to capture stderr and adds table-driven coverage for routing behavior across output modes. |
vhvb1989
left a comment
vhvb1989
left a comment
There was a problem hiding this comment.
Unclear about the issue and the fix.
Can you provide more details about the issue? maybe the actual output and the expected?
cli/azd/pkg/input/console.go
Outdated
| // jsonOutputMode tracks whether JSON output format is active. | ||
| // When true, human-readable messages are routed to handles.Stderr | ||
| // so stdout contains only valid JSON. | ||
| jsonOutputMode bool |
There was a problem hiding this comment.
console uses a io.Writer interface so it does not know what's the expected format or mode. Adding this here contradicts the design. IIRC, the formatter is the one which should be transforming things into JSON, not the console
There was a problem hiding this comment.
Agreed — refactored in c3824da. Renamed jsonOutputMode to structuredOutputMode so the console no longer references a specific output format. The console treats it as a general structured-output flag; the caller (which knows about the formatter) decides what constitutes structured output. All comments and test names updated to reflect format-agnostic semantics.
There was a problem hiding this comment.
The problem was not the name of the variable, lol.
The issue is the console being aware and deciding what to do.
The expected design for what you want to try could be to create a new formatter implementation with the logic you want to follow. Then you would call your formatter like azd something -o json-no-events (assuming you implemented the json-no-events formatter.
IIRC, the VisualStudio-server integration we have, sends each json event to VS json-RPC so VS can stream what is happening to the user.
There was a problem hiding this comment.
Addressed the naming concern in c3824da (renamed jsonOutputMode → structuredOutputMode). The console no longer references JSON specifically — it uses a general structured-output flag set by the caller.
Regarding the broader point: the console already receives the formatter at construction time and uses it for Message() and MessageUxItem(). The structuredOutputMode boolean is computed once from the formatter's kind and cached — the console doesn't re-check the format type on each call. Happy to discuss further if a different layering is preferred.
There was a problem hiding this comment.
@vhvb1989 Gentle ping — any thoughts on the structuredOutputMode refactoring approach? Happy to iterate further if a different layering is preferred.
There was a problem hiding this comment.
@spboyer — following up on my earlier comment about the console not being the right place for this logic.
cmd/container.go (lines 127–131) already handles this exact concern at the right layer:
writer := cmd.OutOrStdout()
// When using JSON formatting, we want to ensure we always write messages from the console to stderr.
if formatter != nil && formatter.Kind() == output.JsonFormat {
writer = cmd.ErrOrStderr()
}The console's Output writer is already set to stderr when --output json is active. This means c.writer inside the console already points to stderr for human-readable messages. If this existing wiring is not achieving what you need, we should understand why and fix it at that layer — not duplicate the decision inside the console.
The structuredOutputMode flag introduced here essentially re-checks what the caller already decided. But it goes further: it also makes the console bypass its own spinner/previewer logic based on the format. That couples the console to formatter semantics it should not know about. The console's job is to write to whatever io.Writer it was given, and the caller (container, VS Server, tests) decides where that writer points. This is the design that keeps the console reusable across contexts (CLI, VS Server, extensions, agents).
If the existing stderr redirect in container.go isn't sufficient for your scenario (e.g., the previewer writes directly to stdout via goterm, bypassing c.writer), the fix should be at the previewer/spinner level — not teaching the console about output formats. Consider the approach I suggested earlier: a new formatter or writer wrapper that encapsulates the routing, keeping the console format-agnostic.
There was a problem hiding this comment.
Addressed in 59e7bb4 — fully removed structuredOutputMode from the console.
The console is now format-agnostic as you described:
messageWriter()simply returnsc.writer(already stderr via container.go's redirect)- Previewer/spinner detect writer redirection via
c.writer != c.defaultWriterinstead of checking format StopPreviewerchecksc.previewer == nil(no format awareness)defaultWriteris set fromhandles.Stdout(real stdout) to enable comparison
The console's job is now just to write to whatever io.Writer it was given. The caller (container.go) decides where that writer points. Thanks for pushing on the right design here.
There was a problem hiding this comment.
I dug into why the container.go stderr redirect isn't enough, and the root cause is the goterm library used by the previewer.
goterm has a hardcoded global output destination (terminal.go:45):
var Output *bufio.Writer = bufio.NewWriter(os.Stdout)tm.Print()/tm.Println() buffer into an internal Screen, and tm.Flush() dumps it to Output — which is always os.Stdout. There is no way to redirect it via an io.Writer. The progressLog (previewer) in progress_log.go calls tm.Print, tm.Println, tm.Flush, tm.MoveCursorUp ~20 times across its lifecycle — all going straight to os.Stdout, bypassing c.writer entirely.
So container.go's redirect does work for everything that flows through c.writer (Message, MessageUxItem, println, StopSpinner's final message). The previewer is the hole.
The fix should be at the previewer level — e.g., make progressLog accept an io.Writer parameter instead of relying on goterm globals, or skip the visual frame rendering when the console is non-interactive / in structured output mode. That keeps the console format-agnostic and fixes the actual leak at its source.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
PR Review - #7393
Route human-readable messages to stderr when --output json is active by @spboyer
Summary
What: Adds a messageWriter() helper to AskerConsole that routes human-readable output (messages, spinner text, previewer content) to stderr when --output json is active. Keeps stdout clean for parseable JSON.
Why: Fixes #7389 - downstream tooling parsing azd JSON output was getting corrupted by interleaved human-readable messages.
Assessment: messageWriter() is the right approach - it centralizes the routing decision and survives runtime writer swaps (e.g., ShowPreviewer temporarily taking over c.writer). The ShowPreviewer/StopPreviewer guards correctly prevent nil-pointer panics. Backward compatibility with non-JSON modes is preserved. Test coverage is solid for the core paths but has gaps for MessageUxItem and the previewer guards.
Findings
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Tests | 0 | 0 | 2 | 0 |
| Logic | 0 | 0 | 0 | 1 |
| Total | 0 | 0 | 2 | 1 |
Key Findings
- [MEDIUM] Tests:
MessageUxItemstderr routing isn't tested - it's a third distinct write path that was changed - [MEDIUM] Tests:
ShowPreviewer/StopPreviewerJSON-mode guards lack test coverage - [LOW] Logic:
StopSpinnerJSON path doesn't have the idempotency guard from the non-JSON path - repeated calls with a non-empty message would double-print
Test Coverage Estimate
- Well covered:
Message,ShowSpinner,StopSpinnerin JSON mode;EnsureBlankLine; backward compat for table/none modes; JSON query filter with stderr - Gaps:
MessageUxItemin JSON mode,ShowPreviewer/StopPreviewerin JSON mode
What's Done Well
messageWriter()is a clean single-responsibility abstractionStopPreviewerguard prevents nil-pointer panic onc.previewer.Stop()when previewer was never created- Tests use
t.Context()and table-driven patterns per repo conventions - Non-JSON modes are completely untouched
2 inline comments below.
|
@vhvb1989 Good point about the boundary. The formatter does handle JSON serialization, but the issue is that non-formatter output (spinner text, status messages, The That said, if you'd prefer this routing to live at a different layer (e.g., a middleware that swaps the Console's writer), happy to restructure. The net effect would be the same — keeping human messages off stdout when |
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
PR Review - #7393 (incremental since d8dd49b)
Route human-readable messages to stderr when --output json is active by @spboyer
Summary
What: Adds test coverage for previously-identified gaps - MessageUxItem stderr routing, ShowPreviewer/StopPreviewer JSON mode guards, EnsureBlankLine routing, and removes time.Sleep from spinner test for deterministic assertions.
Why: Addresses review feedback from @jongio and @copilot-pull-request-reviewer on the previous commit.
Assessment: Tests are well-structured, follow repo conventions (table-driven, t.Context()), and cover the gaps identified in the previous review round. Two minor test isolation improvements possible.
Findings
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Tests | 0 | 0 | 0 | 2 |
| Total | 0 | 0 | 0 | 2 |
What's Done Well
- Clean response to all three feedback points from the previous round
- ShowPreviewer test verifies the returned writer actually routes to stderr by writing through it
- Table-driven pattern with t.Context() matches repo conventions
- Removing time.Sleep is correct - non-interactive spinners are synchronous
2 inline comments below.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
PR Review - #7393 (incremental since a788205)
Route human-readable messages to stderr when --output json is active by @spboyer
Summary
What: Adds a messageWriter() helper to AskerConsole that routes human-readable output (messages, spinner text, previewer content) to stderr when --output json is active. Keeps stdout clean for parseable JSON.
Why: Fixes #7389 - downstream tooling parsing azd JSON output was corrupted by interleaved human-readable messages.
Assessment: The incremental changes (05a6c17) cleanly address the two LOW findings from the previous review round - EnsureBlankLine test isolation and StopPreviewer zero-output assertions. The overall implementation is correct: all c.writer message paths route through messageWriter(), ShowPreviewer/StopPreviewer guards prevent nil panics, and test coverage is solid across all output modes.
Note: @vhvb1989's architectural concern about the console having format awareness is still open. The console already checks formatter.Kind() in multiple places on main, so this PR centralizes rather than introduces the coupling - but the design discussion should be resolved before merge.
Findings
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 |
Zero novel findings after deduplicating against 6 inline threads, 6 review submissions, and 2 conversation comments.
Test Coverage Estimate
- Well covered: Message, MessageUxItem, ShowSpinner, StopSpinner, ShowPreviewer, StopPreviewer, EnsureBlankLine - all in JSON mode with separate stdout/stderr buffers
- Well covered: Non-JSON modes (table, none) verified unchanged
- Well covered: JSON query filter with stderr routing
- No remaining gaps identified
What's Done Well
- messageWriter() is a clean single-responsibility abstraction that survives runtime writer swaps
- Incremental test fixes are minimal and targeted - exactly what was asked for
- StopPreviewer guard prevents nil-pointer panic when previewer was never created
- Test patterns match repo conventions: table-driven, t.Context(), separate stdout/stderr buffers
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Previous comments addressed. Rename to structuredOutputMode cleanly addresses the design feedback. One nit:
- console.go:151 - field comment says "suppressed" but elements are redirected to stderr as plain text
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
…7389) When JSON output is requested, human-readable messages (status text, spinner fallback, informational messages) are routed to stderr so stdout contains only valid, parseable JSON. - Add jsonOutputMode field to AskerConsole to track JSON output state - Add messageWriter() helper that returns stderr in JSON mode - Route Message(), MessageUxItem(), ShowSpinner(), StopSpinner() output to stderr via messageWriter() when JSON mode is active - ShowSpinner emits plain text to stderr instead of being suppressed - StopSpinner emits stop message to stderr instead of being suppressed - Add table-driven tests verifying stderr routing in JSON mode and backward compatibility in table/none modes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ShowPreviewer now returns stderr writer in JSON mode instead of rendering the visual frame via goterm (which writes to stdout). StopPreviewer is a no-op when the previewer was never started. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove time.Sleep from spinner test for deterministic assertions - Add MessageUxItem stderr routing test case - Add ShowPreviewer/StopPreviewer JSON mode test coverage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Azure/azure-dev/sessions/49481e13-7f74-4ef7-84b4-272330b69875 Co-authored-by: jongio <2163001+jongio@users.noreply.github.com>
Rename jsonOutputMode to structuredOutputMode so the console no longer references a specific output format. The console now treats this as a general 'structured output mode' flag — the caller decides what constitutes structured output. Comments and test names updated to reflect format-agnostic semantics. This addresses the design concern that the console (an io.Writer-based abstraction) should not know about the expected format or mode. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per Victor's review, the console should not know about output formats. container.go already sets the Output writer to stderr when --output json is active. The structuredOutputMode flag was redundant. For components that bypass c.writer (previewer/goterm, spinner/yacspin), detect writer redirection by comparing c.writer != c.defaultWriter instead of checking format. This keeps the console reusable across contexts (CLI, VS Server, extensions, agents). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
spboyer
force-pushed
the
feat/json-stderr-routing
branch
from
59e7bb4 to
bf80c1c
Compare
|
@jongio @rajeshkamal5050 ADO CI failures here are not caused by PR code — all tests pass locally. Failing tests ( |
Uh oh!
There was an error while loading. Please reload this page.