fix: add --report-file flag to gh-aw logs to avoid shell redirect failure#40425
Conversation
…lure The activity_report job in agentics-maintenance.yml failed with: "No such file or directory" when trying to redirect stdout to ./.cache/gh-aw/activity-report-logs/report.md Root cause: bash evaluates the `>` redirect before the command runs, so the output directory must already exist. When the cache is not restored the directory doesn't exist yet. Fix: add --report-file flag to `gh-aw logs` that writes the markdown output directly to a file (creating parent directories via constants.DirPermPublic), removing the shell redirect entirely. Updated templates: maintenance_workflow_yaml.go, side_repo_maintenance.go, and agentics-maintenance.yml all use --report-file instead of `>`. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Use an IIFE so defer restores os.Stdout even on panic, and return the f.Close() error so incomplete writes (e.g. disk full) are surfaced. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a --report-file <path> flag to gh aw logs to write the cross-run Markdown report directly to a file, avoiding shell redirection failures when the output directory doesn’t exist yet. It also updates the maintenance workflows to use the new flag instead of >.
Changes:
- Add
--report-fileflag to thelogsCLI and thread it through both normal and--stdinexecution paths. - Implement file creation (including parent directories) for
--format markdownoutput in the logs orchestrator. - Update maintenance workflow generators and the generated workflow YAML to use
--report-filerather than shell redirection.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_command.go | Adds --report-file flag and passes it into logs download options (normal + --stdin). |
| pkg/cli/logs_orchestrator.go | Writes Markdown report to --report-file (mkdir + create) instead of stdout. |
| pkg/workflow/side_repo_maintenance.go | Updates side-repo maintenance workflow command to use --report-file. |
| pkg/workflow/maintenance_workflow_yaml.go | Updates generated maintenance workflow YAML template to use --report-file. |
| .github/workflows/agentics-maintenance.yml | Regenerated workflow output reflecting the --report-file change. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 4
| if err := func() error { | ||
| oldStdout := os.Stdout | ||
| defer func() { os.Stdout = oldStdout }() | ||
| os.Stdout = f | ||
| renderCrossRunReportMarkdown(report) | ||
| return f.Close() | ||
| }(); err != nil { |
| if opts.reportFile != "" { | ||
| if err := os.MkdirAll(filepath.Dir(opts.reportFile), constants.DirPermPublic); err != nil { | ||
| return fmt.Errorf("failed to create report file directory: %w", err) | ||
| } | ||
| f, err := os.Create(opts.reportFile) |
| train, _ := cmd.Flags().GetBool("train") | ||
| format, _ := cmd.Flags().GetString("format") | ||
| reportFile, _ := cmd.Flags().GetString("report-file") | ||
| artifacts, _ := cmd.Flags().GetStringSlice("artifacts") | ||
|
|
| filteredIntegrity, _ := cmd.Flags().GetBool("filtered-integrity") | ||
| train, _ := cmd.Flags().GetBool("train") | ||
| format, _ := cmd.Flags().GetString("format") | ||
| reportFile, _ := cmd.Flags().GetString("report-file") | ||
| artifacts, _ := cmd.Flags().GetStringSlice("artifacts") |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR (#40425). Changed files are production code only: pkg/cli/logs_command.go, pkg/cli/logs_orchestrator.go, pkg/workflow/maintenance_workflow_yaml.go, pkg/workflow/side_repo_maintenance.go, and .github/workflows/agentics-maintenance.yml. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40425 does not have the 'implementation' label and has 32 new lines (≤100) in business logic directories. No custom config present. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — requesting changes on two correctness issues in the implementation.
📋 Key Themes & Highlights
Key Themes
- Goroutine-unsafe stdout swap:
os.Stdout = fis a global mutation; the right fix is to thread anio.WriterthroughrenderCrossRunReportMarkdownand its helpers. - File descriptor leak on panic:
f.Close()is only reached on the happy path inside the IIFE; a panic inrenderCrossRunReportMarkdownleaks the handle. - Silent flag noop:
--report-filewithout--format markdownis silently ignored — needs a validation error. - No regression test: the new
reportFilebranch inrenderLogsOutputhas zero test coverage.
Positive Highlights
- ✅ Root-cause fix is correct — moving directory creation into the binary with
os.MkdirAlleliminates the bash evaluation-order race cleanly. - ✅
f.Close()errors are now propagated, which the shell redirect never did. - ✅ All three callsites (live YAML + two Go templates) updated together — no divergence risk.
- ✅ Flag wired through both the normal and
--stdinpaths.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 70.4 AIC · ⌖ 7.99 AIC · ⊞ 6.9K
| if err := func() error { | ||
| oldStdout := os.Stdout | ||
| defer func() { os.Stdout = oldStdout }() | ||
| os.Stdout = f |
There was a problem hiding this comment.
[/diagnose] os.Stdout is a global process variable — swapping it is not goroutine-safe. Any concurrent goroutine (logger, progress reporter, or parallel test) writing to os.Stdout during this window will write to the file instead of the terminal.
💡 Suggested fix: pass an `io.Writer` instead of swapping the global
Refactor renderCrossRunReportMarkdown (and every renderMarkdown* helper in audit_cross_run_render.go) to accept an io.Writer:
func renderCrossRunReportMarkdown(w io.Writer, report *CrossRunAuditReport) {
fmt.Fprintln(w, "# Audit Report — Cross-Run Analysis")
// ...
}Then the call-site simplifies to:
f, err := os.Create(opts.reportFile)
if err != nil {
return fmt.Errorf("failed to create report file: %w", err)
}
defer f.Close()
renderCrossRunReportMarkdown(f, report)This removes the IIFE entirely, is safe under concurrency, and makes unit tests trivial (pass a bytes.Buffer). The existing tests in audit_cross_run_test.go already use os.Pipe() to capture stdout — those would simplify too.
| defer func() { os.Stdout = oldStdout }() | ||
| os.Stdout = f | ||
| renderCrossRunReportMarkdown(report) | ||
| return f.Close() |
There was a problem hiding this comment.
[/diagnose] File descriptor leak on panic: if renderCrossRunReportMarkdown panics, the defer on line 746 restores os.Stdout but execution never reaches return f.Close() — the file handle is leaked.
💡 Add a deferred close as a safety net
Add defer f.Close() immediately after os.Create to guarantee cleanup regardless of panic or early return. The return f.Close() call on the happy path can remain for error propagation, but defer ensures the handle is released in all other cases:
f, err := os.Create(opts.reportFile)
if err != nil {
return fmt.Errorf("failed to create report file: %w", err)
}
defer f.Close() // safety net for panic or early return
// ...Note: if you adopt the io.Writer refactor suggested above, this IIFE goes away entirely and the issue disappears with it.
| logsCmd.Flags().String("summary-file", "summary.json", "Path to write the summary JSON file relative to output directory (use empty string to disable)") | ||
| logsCmd.Flags().Bool("train", false, "Analyze log patterns across downloaded runs and save pattern weights to drain3_weights.json in the output directory") | ||
| logsCmd.Flags().String("format", "", "Output format: console (decorated tables), tsv (tab-separated), pretty (cross-run report), markdown (cross-run Markdown). Default: compact agent-optimized output") | ||
| logsCmd.Flags().String("report-file", "", "Write --format markdown output directly to this file path instead of stdout (creates parent directories as needed)") |
There was a problem hiding this comment.
[/diagnose] --report-file is silently ignored when --format is not markdown. A user running gh aw logs --report-file out.md (without --format markdown) will get no file and no error — confusing.
💡 Add a validation guard early in RunE
Add a check in RunE (or a PreRunE) before calling DownloadWorkflowLogs:
if reportFile != "" && format != "markdown" {
return fmt.Errorf("--report-file requires --format markdown")
}The flag description could also be made more explicit: "Write Markdown report to this file (requires --format markdown); creates parent directories" to set expectations at --help.
| return nil | ||
| } | ||
| renderCrossRunReportMarkdown(report) | ||
| if opts.reportFile != "" { |
There was a problem hiding this comment.
[/tdd] No test covers the new --report-file code path. Without a regression test, the silent os.Stdout swap and the MkdirAll + os.Create logic can break undetected.
💡 Sketch of a unit test
func TestRenderLogsOutput_ReportFile_WritesMarkdownFile(t *testing.T) {
dir := t.TempDir()
reportPath := filepath.Join(dir, "nested", "report.md")
// build a minimal processedRuns slice ...
err := renderLogsOutput(processedRuns, renderLogsOutputOptions{
format: "markdown",
reportFile: reportPath,
outputDir: dir,
})
require.NoError(t, err)
content, err := os.ReadFile(reportPath)
require.NoError(t, err)
assert.Contains(t, string(content), "# Audit Report", "file should contain markdown header")
}This would have caught the goroutine-safety issue during code review if the test were run with -race, and confirms that parent-directory creation works on first use (cold-cache scenario).
There was a problem hiding this comment.
Non-blocking observations
The core fix is correct — --report-file reliably solves the cold-cache redirect failure. Two new observations beyond the existing review threads:
🔍 Findings summary
Medium — os.Stdout global swap is not goroutine-safe (logs_orchestrator.go:747)
The IIFE restores os.Stdout on panic, but it does not serialize access to the global. Any goroutine that writes to os.Stdout during renderCrossRunReportMarkdown will have its output silently redirected to the report file (or lost after close). In tests that run in parallel this can corrupt report content or test output. The right fix is threading io.Writer through renderCrossRunReportMarkdown and its helpers — see inline comment for a concrete sketch.
Medium — Template tests cannot distinguish --report-file from > path (maintenance_workflow_yaml.go:514, side_repo_maintenance.go:523)
maintenance_workflow_test.go:576 and side_repo_maintenance_integration_test.go:123 only check that the path string appears in the YAML. Both the old broken shell redirect and the new flag pass that assertion. Adding a strings.Contains(yaml, "--report-file") check (and a negative check for ">") would lock in the behavioral fix.
🔎 Code quality review by PR Code Quality Reviewer · 106.4 AIC · ⌖ 7.47 AIC · ⊞ 5.1K
| if err := func() error { | ||
| oldStdout := os.Stdout | ||
| defer func() { os.Stdout = oldStdout }() | ||
| os.Stdout = f |
There was a problem hiding this comment.
os.Stdout = f is a process-global mutation and is not goroutine-safe. Any concurrent goroutine that writes to os.Stdout during renderCrossRunReportMarkdown will redirect its output into the report file, silently corrupting it — or lose output if the file is already closed.
💡 Suggested fix
Thread io.Writer through renderCrossRunReportMarkdown and its six sub-functions instead of swapping the global:
// In renderLogsOutput:
var w io.Writer
if opts.reportFile != "" {
if err := os.MkdirAll(filepath.Dir(opts.reportFile), constants.DirPermPublic); err != nil {
return fmt.Errorf("failed to create report file directory: %w", err)
}
f, err := os.Create(opts.reportFile)
if err != nil {
return fmt.Errorf("failed to create report file: %w", err)
}
defer func() {
if cerr := f.Close(); cerr != nil && err == nil {
err = fmt.Errorf("failed to close report file: %w", cerr)
}
}()
w = f
} else {
w = os.Stdout
}
renderCrossRunReportMarkdown(w, report)Then update renderCrossRunReportMarkdown and all its helpers (renderMarkdownExecutiveSummary, renderMarkdownMetricsTrend, etc.) to accept and forward an io.Writer.
This eliminates the goroutine-safety risk and also fixes the silently-dropped write errors: fmt.Fprintf(f, ...) returns errors that the current design discards because renderCrossRunReportMarkdown has no error return — threading io.Writer makes it straightforward to add one or to use a bufio.Writer + flush check.
| --output ./.cache/gh-aw/activity-report-logs \ | ||
| --format markdown \ | ||
| > ./.cache/gh-aw/activity-report-logs/report.md | ||
| --report-file ./.cache/gh-aw/activity-report-logs/report.md |
There was a problem hiding this comment.
The corresponding test only asserts the path string appears in the YAML, so it cannot distinguish --report-file path from the old > path redirect. A regression that restores the shell redirect would pass undetected.
💡 Suggested fix
Add an explicit assertion for the flag in pkg/workflow/maintenance_workflow_test.go (and its counterpart in side_repo_maintenance_integration_test.go):
if !strings.Contains(yaml, "--report-file ./.cache/gh-aw/activity-report-logs/report.md") {
t.Errorf("activity_report should use --report-file, not shell redirect, got:\n%s", yaml)
}
// Verify the old broken form is gone:
if strings.Contains(yaml, "> ./.cache/gh-aw/activity-report-logs/report.md") {
t.Errorf("activity_report must not use shell redirect for report.md, got:\n%s", yaml)
}The path-only check (strings.Contains(yaml, "report.md")) will always pass regardless of how the path is used in the command.
|
Hey One thing that would help land this cleanly:
If you'd like a hand, you can assign this prompt to your coding agent:
|
|
@copilot review all comments and address unresolved review feedback.
|
|
Address the requested changes and missing tests, then re-request review.
|
|
@copilot review all comments and address unresolved review feedback.
|
|
@copilot run pr-finisher skill |
…add test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — addressed all four review threads:
All local checks pass ( |
Bash evaluates
>redirects before the command runs. When the activity report cache is cold,./.cache/gh-aw/activity-report-logs/doesn't exist yet, so the redirect fails immediately withNo such file or directory— beforegh-aw logsever runs or creates the output directory.Changes
pkg/cli/logs_command.go— adds--report-file <path>flag togh-aw logs; wired through both normal and--stdinpathspkg/cli/logs_orchestrator.go— when--format markdown+--report-fileare combined, creates the file (and parent dirs) directly instead of writing to stdout; uses an IIFE withdeferto guaranteeos.Stdoutrestoration on panic; propagatesf.Close()errorsmaintenance_workflow_yaml.go,side_repo_maintenance.go,agentics-maintenance.yml— replace the>redirect with--report-fileBefore:
After:
${GH_AW_CMD_PREFIX} logs \ --output ./.cache/gh-aw/activity-report-logs \ --format markdown \ --report-file ./.cache/gh-aw/activity-report-logs/report.md