Skip to content

fix: add --report-file flag to gh-aw logs to avoid shell redirect failure #40425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 5 commits into from
Jun 20, 2026
Merged

fix: add --report-file flag to gh-aw logs to avoid shell redirect failure #40425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c03bf95
fix: add --report-file flag to gh-aw logs to avoid shell redirect fai…
Copilot Jun 20, 2026
f23c6ea
fix: safe stdout restore and propagate close error in --report-file
Copilot Jun 20, 2026
6e20994
fix: defer f.Close() for panic safety, add --report-file validation, …
Copilot Jun 20, 2026
7832c59
refactor: extract validateReportFileFlags helper to avoid duplication
Copilot Jun 20, 2026
0b4c547
Merge branch 'main' into copilot/fix-script-file-output
pelikhan Jun 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/agentics-maintenance.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ jobs:
--count 100 \
--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

- name: Save activity report logs cache
if: ${{ always() }}
Expand Down
29 changes: 29 additions & 0 deletions pkg/cli/logs_command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ Downloaded artifacts include (when using --artifacts all):
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")

Comment on lines 154 to 158
if engine != "" {
Expand All @@ -164,6 +165,10 @@ Downloaded artifacts include (when using --artifacts all):
}
}

if err := validateReportFileFlags(reportFile, format, jsonOutput); err != nil {
return err
}

return DownloadWorkflowLogsFromStdin(cmd.Context(), StdinLogsOptions{
RunURLs: runURLs,
OutputDir: outputDir,
Expand All @@ -182,6 +187,7 @@ Downloaded artifacts include (when using --artifacts all):
FilteredIntegrity: filteredIntegrity,
Train: train,
Format: format,
ReportFile: reportFile,
ArtifactSets: artifacts,
})
}
Expand Down Expand Up @@ -268,6 +274,7 @@ Downloaded artifacts include (when using --artifacts all):
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")
Comment on lines 274 to 278
cacheBefore, _ := cmd.Flags().GetString("cache-before")
if !cmd.Flags().Changed("cache-before") {
Expand Down Expand Up @@ -307,6 +314,10 @@ Downloaded artifacts include (when using --artifacts all):
}
}

if err := validateReportFileFlags(reportFile, format, jsonOutput); err != nil {
return err
}

logsCommandLog.Printf("Executing logs download: workflow=%s, count=%d, engine=%s, train=%v, cache_before=%s", workflowName, count, engine, train, cacheBefore)

return DownloadWorkflowLogs(cmd.Context(), LogsDownloadOptions{
Expand All @@ -333,6 +344,7 @@ Downloaded artifacts include (when using --artifacts all):
FilteredIntegrity: filteredIntegrity,
Train: train,
Format: format,
ReportFile: reportFile,
ArtifactSets: artifacts,
After: cacheBefore,
})
Expand Down Expand Up @@ -361,6 +373,7 @@ Downloaded artifacts include (when using --artifacts all):
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)")

@github-actions github-actions Bot Jun 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

logsCmd.Flags().Int("last", 0, "Alias for --count: number of recent runs to download")
logsCmd.Flags().StringSlice("artifacts", []string{"usage"}, "Artifact sets to download (default: usage). Use 'all' for everything, or comma-separate sets. Valid sets: "+validArtifactSets)
logsCmd.Flags().String("cache-before", "", "(Cache eviction) Evict locally cached run folders for runs before this date, prior to downloading. Accepts deltas like -1d, -1w, -1mo (or explicit day counts like -30d), or an absolute date YYYY-MM-DD. Unlike --start-date, this only clears local cache and does not filter which runs are fetched.")
Expand Down Expand Up @@ -450,3 +463,19 @@ func repoIsLocal(repo string) bool {
}
return strings.EqualFold(ownerRepo, currentRepo)
}

// validateReportFileFlags returns an error if --report-file is combined with an
// incompatible flag. --report-file only takes effect for --format markdown output
// and is bypassed when --json is set.
func validateReportFileFlags(reportFile, format string, jsonOutput bool) error {
if reportFile == "" {
return nil
}
if format != "markdown" {
return errors.New("--report-file requires --format markdown")
}
if jsonOutput {
return errors.New("--report-file cannot be used with --json")
}
return nil
}
31 changes: 30 additions & 1 deletion pkg/cli/logs_orchestrator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ type LogsDownloadOptions struct {
Format string
ArtifactSets []string
After string
ReportFile string
}

func shouldStopPagination(totalFetched, batchSize int) bool {
Expand Down Expand Up @@ -634,6 +635,7 @@ func DownloadWorkflowLogs(ctx context.Context, opts LogsDownloadOptions) error {
outputDir: outputDir,
summaryFile: summaryFile,
format: format,
reportFile: opts.ReportFile,
jsonOutput: jsonOutput,
toolGraph: toolGraph,
train: train,
Expand All @@ -648,6 +650,7 @@ type renderLogsOutputOptions struct {
outputDir string
summaryFile string
format string
reportFile string
jsonOutput bool
toolGraph bool
train bool
Expand Down Expand Up @@ -730,7 +733,31 @@ func renderLogsOutput(processedRuns []ProcessedRun, opts renderLogsOutputOptions
renderLogsArtifactHint(os.Stderr, logsData.Message)
return nil
}
renderCrossRunReportMarkdown(report)
if opts.reportFile != "" {

@github-actions github-actions Bot Jun 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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).

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)
Comment on lines +736 to +740
if err != nil {
return fmt.Errorf("failed to create report file: %w", err)
}
if err := func() (retErr error) {
defer func() {
if cerr := f.Close(); cerr != nil && retErr == nil {
retErr = cerr
}
}()
oldStdout := os.Stdout
defer func() { os.Stdout = oldStdout }()
os.Stdout = f

@github-actions github-actions Bot Jun 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

@github-actions github-actions Bot Jun 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

renderCrossRunReportMarkdown(report)
return nil
}(); err != nil {
return fmt.Errorf("failed to write report file: %w", err)
}
} else {
renderCrossRunReportMarkdown(report)
}
renderLogsArtifactHint(os.Stderr, logsData.Message)
return nil

Expand Down Expand Up @@ -794,6 +821,7 @@ type StdinLogsOptions struct {
FilteredIntegrity bool
Train bool
Format string
ReportFile string
// ArtifactSets defaults to nil (download all artifacts) when this API is used
// programmatically. The CLI passes ["usage"] to match the logs command default.
ArtifactSets []string
Expand Down Expand Up @@ -1110,6 +1138,7 @@ func DownloadWorkflowLogsFromStdin(ctx context.Context, opts StdinLogsOptions) e
outputDir: opts.OutputDir,
summaryFile: opts.SummaryFile,
format: opts.Format,
reportFile: opts.ReportFile,
jsonOutput: opts.JSONOutput,
toolGraph: opts.ToolGraph,
train: opts.Train,
Expand Down
45 changes: 45 additions & 0 deletions pkg/cli/logs_report_file_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
//go:build !integration

package cli

import (
"os"
"path/filepath"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// TestRenderLogsOutputReportFileWritesMarkdownToFile verifies that when --report-file
// is set with --format markdown, the markdown report is written to the file and
// stdout receives no markdown report content.
func TestRenderLogsOutputReportFileWritesMarkdownToFile(t *testing.T) {
tmpDir := t.TempDir()
reportFile := filepath.Join(tmpDir, "report.md")

processedRuns := []ProcessedRun{{
Run: WorkflowRun{
DatabaseID: 1,
Status: "completed",
WorkflowName: "logs",
CreatedAt: time.Now(),
},
}}

stdout, _ := captureOutput(t, func() error {
return renderLogsOutput(processedRuns, renderLogsOutputOptions{
outputDir: tmpDir,
format: "markdown",
reportFile: reportFile,
artifactFilter: []string{"usage"},
})
})

assert.NotContains(t, stdout, "# Audit Report", "stdout should not contain the markdown report when --report-file is set")

content, err := os.ReadFile(reportFile)
require.NoError(t, err, "report file should be created by renderLogsOutput")
assert.Contains(t, string(content), "# Audit Report — Cross-Run Analysis", "report file should contain the markdown report header")
}
2 changes: 1 addition & 1 deletion pkg/workflow/maintenance_workflow_yaml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ jobs:
--count 100 \
--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

@github-actions github-actions Bot Jun 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


- name: Save activity report logs cache
if: ${{ always() }}
Expand Down
2 changes: 1 addition & 1 deletion pkg/workflow/side_repo_maintenance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,7 @@ jobs:
--count 100 \
--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

- name: Save activity report logs cache
if: ${{ always() }}
Expand Down
Loading