pkg/cli: add io.Writer params to renderLogs* formatters; kill captureOutput OS-swap#47372
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds injectable writers to five log formatters, reducing process-global stdout capture in tests.
Changes:
- Adds
...ToWritervariants with stdout-compatible wrappers. - Generalizes artifact hints to
io.Writer. - Converts formatter tests to buffers and enables parallel execution.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_report.go |
Adds JSON writer rendering. |
pkg/cli/logs_orchestrator_render.go |
Generalizes hint output. |
pkg/cli/logs_format_tsv.go |
Adds TSV writer variants. |
pkg/cli/logs_format_compact.go |
Adds compact writer variants. |
pkg/cli/logs_output_hint_test.go |
Uses buffered compact output. |
pkg/cli/logs_json_test.go |
Captures JSON in a buffer. |
pkg/cli/logs_json_clean_test.go |
Tests buffered JSON output. |
pkg/cli/logs_format_tsv_test.go |
Uses parallel buffered tests. |
pkg/cli/logs_empty_runs_test.go |
Uses parallel buffered JSON testing. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Medium
| // renderLogsJSONToWriter outputs the logs data as JSON to w. | ||
| // When verbose is false, audit-heavy fields are stripped for compact agentic consumption. | ||
| func renderLogsJSON(data LogsData, verbose bool) error { | ||
| func renderLogsJSONToWriter(w io.Writer, data LogsData, verbose bool) error { |
PR Triage
Score breakdown: Impact 22/50 · Urgency 18/30 · Quality 18/20 Rationale: Adds injectable io.Writer to log formatters, eliminating process-global stdout capture in tests. Pure internal refactor — no user-facing behavior changes. 9 files, all in pkg/cli. CI nearly all green (lint and Windows Integration still in progress but build/test pass). Good description, tests updated. Candidate for auto-merge once CI completes. Labels applied: pr-type:refactor · pr-risk:low · pr-priority:medium · pr-action:auto_merge · pr-agent:copilot
|
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…sistency) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Added |
|
@copilot please run the
|
|
|
@copilot please run the
|
All code changes are complete and local validation passes. The one remaining item is resolving the |
The six
renderLogs*formatters inpkg/cli/wrote directly toos.Stdout, forcing tests to use acaptureOutputhelper that swapsos.Stdoutat the OS level behind a mutex and cannot be used witht.Parallel().Changes
New
...ToWritervariants (matching existingrenderCrossRunReportMarkdownToWriterconvention)renderLogsTSVToWriter(w io.Writer, data LogsData)/renderLogsTSVVerboseToWriterrenderLogsCompactToWriter(w io.Writer, data LogsData)/renderLogsCompactVerboseToWriterrenderLogsJSONToWriter(w io.Writer, data LogsData, verbose bool) errorOriginal functions kept as thin
os.Stdoutwrappers — no call-site changes needed.renderLogsArtifactHintwidened from*os.Filetoio.Writer.Tests — five test files converted from
captureOutput/os.Stdoutpipe-swap to directbytes.Bufferwrites;t.Parallel()added:logs_format_tsv_test.gologs_output_hint_test.gologs_json_test.gologs_json_clean_test.go(TestJSONOutputNotCorruptedByStderr)logs_empty_runs_test.goRun: https://github.com/github/gh-aw/actions/runs/29952896403