Add concurrent multi-workflow logs reports - #57976
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
🏗️ ADR required — draft added for multi-workflow logs designI generated a draft ADR at Why this gate triggered
Evidence used
Next action
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new multi-workflow path introduces at least two merge-blocking regressions: local workflow arguments can now be misparsed as cross-repo targets, and the worker goroutines capture the loop variable so results can be attributed to the wrong target.
Blocking themes
- Target parsing is no longer behavior-safe. The new
owner/repo/workflowdetection accepts too many relative paths and can silently redirect a local workflow argument into remote lookup. - The concurrency fan-out is not safe as written. Each worker closes over the shared range variable, so multi-target downloads can duplicate the last target or scramble per-target errors and continuations.
Those are correctness issues in the core command path, so this should not merge before they are fixed.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 53.8 AIC · ⌖ 7.48 AIC · ⊞ 21.8K
Comment /review to run again
| return logsWorkflowTarget{workflowName: workflowName}, err | ||
| } | ||
|
|
||
| func splitCrossRepoWorkflowTarget(arg string) (string, string, bool) { |
There was a problem hiding this comment.
This cross-repo splitter is too eager: any non-existent path with three slash-separated segments is treated as owner/repo/workflow, so a local workflow under a nested directory like docs/foo/bar.md stops being resolved locally and gets sent to a remote repo lookup instead.
💡 Why this blocks
splitCrossRepoWorkflowTarget() only excludes absolute paths, .github/..., and paths that already exist on disk. That means valid relative workflow paths in other directories are reinterpreted as cross-repo targets as soon as they contain two / separators. This is a behavior break in the CLI parser, not just a validation edge case, and it will route the command at the wrong repository instead of failing or using the local file the caller meant.
Tighten the parser so cross-repo mode only activates for unambiguous repo slugs, or fall back to local resolution before treating a token as owner/repo/workflow.
| sem := make(chan struct{}, workerCount) | ||
| perTargetDownloads := max(1, getMaxConcurrentDownloads()/workerCount) | ||
| for _, target := range targets { | ||
| wg.Go(func() { |
There was a problem hiding this comment.
This goroutine closes over the range variable target, so concurrent collectors can all observe the same final target and duplicate or misattribute downloads.
💡 Why this blocks
for _, target := range targets { wg.Go(func() { ... target ... }) } captures the loop variable by reference. In Go, that means every worker shares the same variable storage unless you rebind it inside the loop first. The failure mode here is nasty: output directories, repo overrides, errors, and continuations can all be attached to the wrong workflow target, which defeats the whole point of the multi-target orchestration.
Rebind inside the loop before spawning the goroutine, e.g. target := target, and then use that stable value in the closure.
There was a problem hiding this comment.
🟡 Changes recommended
Host-aware throttling, collision-safe cache isolation, and partial-result propagation must be corrected for reliable concurrent reporting.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds concurrent, resilient log reporting across multiple workflows and repositories.
Changes:
- Parses multiple local, cross-repository, and host-qualified workflow targets.
- Collects targets concurrently with bounded artifact downloads and merged reporting.
- Adds per-target continuations, cache directories, tests, and release notes.
File summaries
| File | Description |
|---|---|
.changeset/minor-multi-workflow-logs.md |
Records the minor feature release. |
cmd/gh-aw/argument_syntax_test.go |
Updates repeatable argument expectations. |
pkg/cli/logs_command.go |
Parses and dispatches multiple targets. |
pkg/cli/logs_command_test.go |
Tests target parsing and help text. |
pkg/cli/logs_multi.go |
Implements concurrent collection and merging. |
pkg/cli/logs_multi_test.go |
Tests concurrency and partial failures. |
pkg/cli/logs_orchestrator.go |
Separates collection from rendering. |
pkg/cli/logs_orchestrator_download.go |
Adds shared throttling and download limits. |
pkg/cli/logs_orchestrator_render.go |
Adds per-target continuations to reports. |
pkg/cli/logs_orchestrator_types.go |
Defines internal multi-target state. |
pkg/cli/logs_rate_limit.go |
Adds a shared rate-limit gate. |
pkg/cli/logs_rate_limit_test.go |
Tests cancellable gate waiting. |
pkg/cli/logs_report.go |
Defines multi-target continuation output. |
pkg/cli/logs_run_processor.go |
Supports per-target concurrency bounds. |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 5
- Review effort level: Balanced
| if target.repoOverride == "" { | ||
| return filepath.Join(root, workflowDir) | ||
| } | ||
| repoDir := "repo-" + stringutil.SanitizeForFilename(target.repoOverride) |
| case <-ctx.Done(): | ||
| return contextCause(ctx) | ||
| } | ||
| return checkAndWaitForRateLimit(ctx, verbose) |
| } | ||
|
|
||
| results := collectLogsTargets(ctx, opts, targets) | ||
| processedRuns, continuations, timeoutReached, allErrors := mergeLogsTargetResults(results, initialErrors) |
| return nil | ||
| } | ||
| if err := checkAndWaitForRateLimit(ctx, verbose); err != nil { | ||
| if err := checkAndWaitForRateLimitShared(ctx, verbose); err != nil { |
| targetOpts.OutputDir = logsTargetOutputDir(opts.OutputDir, target) | ||
| targetOpts.SummaryFile = "" | ||
| targetOpts.Train = false | ||
| targetOpts.SuppressRender = true |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /tdd, and /diagnosing-bugs to the new multi-workflow log collection logic (logs_multi.go, logs_command.go).
📋 Key Themes & Highlights
Key Themes
- Cross-platform path handling:
displayName()mixes OS-native path joining with the forward-slashowner/repo/workflowvocabulary used elsewhere, risking inconsistent messages on Windows. - Concurrency budget fairness: the static per-target download concurrency split (
getMaxConcurrentDownloads()/workerCount) doesn't rebalance when targets finish at different speeds; no test covers uneven completion. - Ambiguous target resolution:
splitCrossRepoWorkflowTargetfalls back toos.Statto distinguish cross-repo targets from local paths — a filesystem-state-dependent heuristic that isn't covered by a collision-case test.
Positive Highlights
- ✅ Clean separation of per-target orchestration (
logs_multi.go) from single-target logic (logs_orchestrator.go), matching the existingLogsDownloadOptionsinterface style well. - ✅ Good resilience design: partial failures across targets are collected and reported without discarding successful results, and this is well tested in
TestDownloadWorkflowLogsForTargetsConcurrentAndResilient. - ✅ Shared rate-limit gate (
checkAndWaitForRateLimitShared) is a sensible way to stagger concurrent workers against a single API quota, with cancellation correctly tested.
None of the findings are blocking; they're refinement suggestions for polish and edge-case robustness.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 75.9 AIC · ⌖ 14.9 AIC · ⊞ 10.3K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/logs_multi.go:161
[/codebase-design] displayName() uses filepath.Join(t.repoOverride, t.workflowName), which normalizes separators to the OS path separator. On Windows this turns owner/repo + .github/workflows/x.yml into a backslash-joined string, inconsistent with the forward-slash owner/repo/workflow vocabulary used everywhere else in this PR (flags, docs, splitCrossRepoWorkflowTarget).
<details>
<summary>💡 Suggested fix</summary>
func (t logsWorkflowTarget) displayName() string {
if …
</details>
<details><summary>pkg/cli/logs_multi.go:412</summary>
**[/tdd]** `perTargetDownloads := max(1, getMaxConcurrentDownloads()/workerCount)` divides the total download budget evenly per workflow worker, but if a fast target finishes early its unused concurrency slots aren't reclaimed by slower targets — the pool sits under-utilized for the remainder of the run. No test exercises this uneven-completion scenario (only the equal-and-immediate-completion path is covered in `logs_multi_test.go`).
<details>
<summary>💡 Suggested test</summary>
Add a test …
</details>
<details><summary>pkg/cli/logs_command.go:292</summary>
**[/diagnosing-bugs]** `os.Stat(arg)` is used to disambiguate a cross-repo target (`owner/repo/workflow`) from a local file path, but this makes CLI argument parsing depend on the invoking directory's filesystem state. A workflow name that happens to collide with an existing local file/dir (e.g. a repo checked out with a folder literally named `owner`) would silently be treated as local instead of cross-repo, with no test covering that ambiguity.
<details>
<summary>💡 Suggestion</summary>
Con…
</details>
There was a problem hiding this comment.
Reviewed the new concurrent multi-workflow logs feature (pkg/cli/logs_multi.go + supporting orchestrator/rate-limit changes). Architecture is solid: bounded worker concurrency via semaphore, per-target output-dir isolation to avoid run-ID collisions, shared rate-limit gate with context-cancellation support, and good test coverage proving concurrency and partial-failure resilience.
One real correctness gap found: the multi-target path never aggregates/forwards countLimitReached, so the dateRangeCoverageWarning staleness check is effectively dead for combined multi-workflow reports (see inline comment on logs_multi.go). This could silently mislead users about data completeness within an explicit date range, so I'm requesting changes to fix it — it should be a small, low-risk fix mirroring the single-target path.
No other blocking issues found (build could not be run locally — sandboxed environment lacks network access to fetch the pinned Go 1.26.7 toolchain — so this review is based on careful manual reading of the diff).
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 143.7 AIC · ⌖ 14.8 AIC · ⊞ 8.3K
| checkStaleness: true, | ||
| suppressRender: opts.SuppressRender, | ||
| continuations: continuations, | ||
| }) |
There was a problem hiding this comment.
mergeLogsTargetResults (lines 130-156) tracks timeoutReached across targets but never aggregates countLimitReached from targetResult.result.countLimitReached, and this renderLogsOutputOptions{} literal never sets countLimitReached (defaults to false).
Compare with the single-target path in logs_orchestrator.go (DownloadWorkflowLogs), which explicitly passes countLimitReached: result.countLimitReached into the same options struct. Here, checkStaleness: true is set but dateRangeCoverageWarning(processedRuns, opts.startDate, opts.endDate, opts.countLimitReached) (see logs_orchestrator_render.go:77 and logs_orchestrator.go:121) requires partial (i.e. countLimitReached) to be true to ever emit its coverage warning. For multi-target reports this means the warning is silently dead: if a per-target --count limit is hit within an explicit start/end date range, users get no signal that the combined report may be missing runs for that range, even though the same situation would be flagged for a single-target run.
Suggest: have mergeLogsTargetResults also OR together targetResult.result.countLimitReached (similar to timeoutReached), return it, and pass it here as countLimitReached: countLimitReached.
@copilot please address this.
|
@copilot Please address the active blocking feedback before this PR moves forward.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the blocking multi-target feedback in commit |
|
🎉 This pull request is included in a new release. Release: |
gh aw logspreviously handled one workflow and repository per invocation. This change supports resilient, concurrent reporting across workflows in multiple repositories.Targeting
owner/repo/workflowand full workflow file paths.--repobehavior.Downloads
Reporting and caching