feat: add --stdin flag to logs and audit commands#29170
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/da607e9a-8783-4979-8e69-79acf091003f Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds --stdin support to gh aw logs and gh aw audit, allowing workflow run IDs/URLs to be streamed via stdin to bypass API run discovery.
Changes:
- Added
readRunIDsFromStdin(io.Reader)helper + unit tests for trimming/blank/comment handling. - Added
--stdinflag handling tologsandauditcommands with mutual-exclusion validation vs positional args. - Refactored logs rendering into a shared
renderLogsOutputhelper and introducedDownloadWorkflowLogsFromStdinorchestration.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/stdin.go | New stdin reader helper for run IDs/URLs. |
| pkg/cli/stdin_test.go | Unit tests for stdin parsing edge cases. |
| pkg/cli/logs_orchestrator.go | Extracts output rendering helper; adds stdin-based logs download path. |
| pkg/cli/logs_command.go | Adds --stdin flag and routes to stdin-based logs orchestration. |
| pkg/cli/audit.go | Adds --stdin flag and updates argument validation to allow stdin-fed inputs. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/cli/logs_command.go:153
- In the
--stdinpath, the--aftercache-cleanup flag is never read/applied (it is only passed toDownloadWorkflowLogsin the non-stdin path). As a result,gh aw logs --stdin --after ...silently ignores cleanup. Consider either (a) reading--afterhere and performing the same cleanup logic asDownloadWorkflowLogs, (b) adding anafterparameter toDownloadWorkflowLogsFromStdin, or (c) erroring/warning when--afteris set alongside--stdin.
return DownloadWorkflowLogsFromStdin(cmd.Context(), runURLs, outputDir, engine, repoOverride, verbose, toolGraph, noStaged, firewallOnly, noFirewall, parse, jsonOutput, timeout, summaryFile, safeOutputType, filteredIntegrity, train, format, artifacts)
- Files reviewed: 5/5 changed files
- Comments generated: 5
| if owner == "" { | ||
| owner = ownerOverride | ||
| repo = repoNameOverride | ||
| } |
There was a problem hiding this comment.
When stdin provides a numeric run ID (no embedded owner/repo) and --repo is not set, ownerOverride/repoNameOverride remain empty and this path falls back to fetchWorkflowRunMetadata with no repo context. That typically fails with a generic "Could not resolve" error from gh api. It would be more user-friendly to detect this case (owner=="" and override empty) and return an explicit error telling the user to pass --repo owner/repo (or provide full run URLs).
| } | |
| } | |
| if owner == "" { | |
| return fmt.Errorf("run %q does not include repository information; pass --repo owner/repo or provide full run URLs", rawURL) | |
| } |
| // Download artifacts for all runs concurrently | ||
| downloadResults := downloadRunArtifactsConcurrent(ctx, runs, outputDir, verbose, len(runs), repoOverride, artifactFilter) |
There was a problem hiding this comment.
Artifact downloads here are driven only by the single repoOverride string passed into downloadRunArtifactsConcurrent, but stdin inputs can include full run URLs (and GHES hosts) that may not match the current repo or --repo. Because downloadRunArtifactsConcurrent ultimately calls gh run download without per-run -R/--hostname, runs from other repos/hosts will fail to download even though metadata parsing succeeded. Consider extending the download pipeline to carry owner/repo/host per run (or explicitly reject mixed/implicit repo contexts and require --repo when stdin inputs aren't for the current repo).
| stdin, _ := cmd.Flags().GetBool("stdin") | ||
|
|
||
| // When --stdin is provided, read run IDs/URLs from stdin and bypass GitHub API discovery. | ||
| if stdin { | ||
| if len(args) > 0 { | ||
| return errors.New(console.FormatErrorWithSuggestions( | ||
| "positional arguments are not allowed with --stdin", | ||
| []string{"Remove the workflow name argument, or omit --stdin to use the normal discovery mode"}, | ||
| )) | ||
| } | ||
| logsCommandLog.Printf("Reading run IDs from stdin") |
There was a problem hiding this comment.
The new --stdin execution path adds non-trivial argument validation (mutual exclusion with positional args) and stdin parsing behavior, but there are no command-level tests asserting these behaviors. Since this repo already has CLI flag/command tests (e.g., pkg/cli/logs_command_test.go), consider adding coverage to verify: logs --stdin rejects positional args, reads multiple lines (ignoring blanks/comments), and wires through to the stdin download path.
This issue also appears on line 153 of the same file.
| if stdin { | ||
| if len(args) > 0 { | ||
| return errors.New(console.FormatErrorWithSuggestions( | ||
| "positional arguments are not allowed with --stdin", | ||
| []string{"Remove the run ID arguments, or omit --stdin to use positional arguments"}, |
There was a problem hiding this comment.
The --stdin branch adds new validation and stdin ingestion for audit (mutual exclusion with positional args, empty-stdin handling), but there are no unit tests covering these CLI behaviors. Since the repo already tests many command surfaces, consider adding tests that verify: audit --stdin rejects positional args, errors when neither args nor stdin are provided, and correctly uses stdin lines as args for both single-run and diff modes.
| if repoOverride != "" { | ||
| parts := strings.SplitN(repoOverride, "/", 2) | ||
| if len(parts) != 2 || parts[0] == "" || parts[1] == "" { | ||
| return fmt.Errorf("invalid repository format '%s': expected 'owner/repo'", repoOverride) | ||
| } |
There was a problem hiding this comment.
repoOverride parsing here only supports owner/repo, but the --repo flag advertises [HOST/]owner/repo (see pkg/cli/flags.go:26). This means values like github.example.com/owner/repo will be mis-parsed and can lead to incorrect API/CLI calls. Consider parsing 2 or 3 path segments (host optional) and updating the validation error message accordingly; also plumb the parsed host through to the metadata/artifact download steps.
… commands Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (488 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification Details
Flagged Tests — Requires ReviewNo tests failed the quality threshold. One observation is noted below. i️ Missing error-path coverage —
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@copilot review all comments |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/5176ce98-d935-4b8c-a868-018e6e7df922 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All five reviewer comments addressed in commit aa68af6:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Summary
Adds a
--stdinflag to bothgh aw logsandgh aw auditcommands that reads workflow run IDs or URLs from stdin (one per line), bypassing the GitHub API run-discovery step.Usage
Stdin format:
#are ignoredChanges
pkg/cli/stdin.go— NewreadRunIDsFromStdin()helper that reads and trims lines from anyio.Readerpkg/cli/stdin_test.go— Unit tests covering all edge cases (blank lines, comments, URLs, trimming)pkg/cli/logs_orchestrator.go— ExtractedrenderLogsOutput()shared helper fromDownloadWorkflowLogs; addedDownloadWorkflowLogsFromStdin()that fetches metadata per run ID and processes them through the same filter/render pipeline;--reponow supports[HOST/]owner/repoformat with the parsed host plumbed through to metadata fetch and artifact downloadpkg/cli/logs_run_processor.go—downloadRunArtifactsConcurrentnow parses[HOST/]owner/repofrom the global--repooverride and, when no override is set, derives per-run owner/repo/host fromrun.URLso stdin inputs from different repositories or GHES instances download artifacts correctlypkg/cli/logs_command.go— Added--stdinflag; when set, reads from stdin and callsDownloadWorkflowLogsFromStdin; positional workflow-name arg is disallowed when--stdinis usedpkg/cli/audit.go— Added--stdinflag; changedcobra.MinimumNArgs(1)tocobra.ArbitraryArgswith custom validation that accepts either positional args or--stdin(but not both)pkg/cli/logs_command_test.go— Tests for--stdinflag registration and positional-arg conflict validationpkg/cli/audit_test.go— Tests for--stdinflag registration, positional-arg conflict, and no-args errorNotes
--stdinand positional arguments are mutually exclusive on both commandslogs --stdin, date/count/workflow-name filtering flags are ignored (the provided run list is used directly); content filters (--engine,--firewall,--safe-output, etc.) still apply--repo [HOST/]owner/repois required; omitting it returns a clear error messagehttps://github.com/owner/repo/actions/runs/…) carry their own repo context and do not require--repo; mixed inputs from different repos are supported