refactor: split logs_orchestrator.go (1284 lines) into 5 focused modules#44051
Conversation
- Extract types into logs_orchestrator_types.go (87 lines) - Extract filter logic and ProcessedRun builder into logs_orchestrator_filters.go (184 lines) including new applyRunFilters and buildProcessedRun helpers that eliminate ~250 lines of duplication - Extract rendering into logs_orchestrator_render.go (158 lines) - Extract stdin processing into logs_orchestrator_stdin.go (229 lines) - Trim logs_orchestrator.go from 1284 to 603 lines - Add logs_orchestrator_filters_test.go with tests for new helpers Closes #44036 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Refactors the logs download orchestrator by splitting the previously monolithic pkg/cli/logs_orchestrator.go into focused modules, while deduplicating run-filtering and ProcessedRun construction logic shared between the normal and stdin-driven download paths.
Changes:
- Split orchestrator responsibilities into separate modules (types, filters, rendering, stdin path) while keeping the public API intact.
- Consolidated duplicated filtering +
ProcessedRunconstruction into shared helpers (applyRunFilters,buildProcessedRun). - Added unit tests covering the new shared helpers.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_orchestrator.go | Removes duplicated filter/run-building/render/stdin logic; keeps main pagination loop and DownloadWorkflowLogs orchestration. |
| pkg/cli/logs_orchestrator_types.go | Extracts public option structs and internal option/helper types into a dedicated types module. |
| pkg/cli/logs_orchestrator_filters.go | Introduces shared run-filtering and ProcessedRun construction helpers to eliminate duplication. |
| pkg/cli/logs_orchestrator_render.go | Moves output rendering (renderLogsOutput, renderLogsArtifactHint) into a focused rendering module. |
| pkg/cli/logs_orchestrator_stdin.go | Moves stdin-driven download path (DownloadWorkflowLogsFromStdin) into its own module and uses shared helpers. |
| pkg/cli/logs_orchestrator_filters_test.go | Adds unit tests for the newly centralized filtering and ProcessedRun construction helpers. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Low
| // runFilterOpts bundles the filter flags passed to applyRunFilters. | ||
| type runFilterOpts struct { | ||
| engine string | ||
| noStaged bool | ||
| firewallOnly bool | ||
| noFirewall bool | ||
| safeOutputType string | ||
| filteredIntegrity bool | ||
| } | ||
|
|
| // Add failed jobs to error count. | ||
| if failedJobCount, err := fetchJobStatuses(run.DatabaseID, verbose); err == nil { | ||
| run.ErrorCount += failedJobCount | ||
| if verbose && failedJobCount > 0 { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Added %d failed jobs to error count for run %d", failedJobCount, run.DatabaseID))) | ||
| } | ||
| } |
| func TestBuildProcessedRun(t *testing.T) { | ||
| t.Run("basic fields are propagated", func(t *testing.T) { |
| skip := applyRunFilters(result, runFilterOpts{safeOutputType: "create-issue"}, false) | ||
| assert.True(t, skip) | ||
| }) | ||
| } |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (929 new lines in 📄 Draft ADR committed:
📋 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 MatterADRs 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 Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (6 tests)
|
Review SummaryApplied 🔴 Issues to address (3)
✅ What's working well
@copilot please address the review comments above.
|
![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 /tdd and /codebase-design — requesting changes on three issues.
📋 Key Themes & Highlights
Issues Found
- Missing test coverage:
filteredIntegrityfilter has no test — every other filter flag inrunFilterOptsis exercised but this one is absent (see inline comment) - Hardcoded
/tmppath: onebuildProcessedRuntest case uses/tmp/test-runinstead oft.TempDir(), inconsistent with the rest of the file - Silent behaviour change:
buildProcessedRunnow emits a verbose log forfailedJobCountthat the oldDownloadWorkflowLogsFromStdindid not — the stdin path observable output changes
Positive Highlights
- ✅ Excellent deduplication — the ~150-line filter block and
ProcessedRun{}construction that lived in both download paths are now a single shared helper - ✅ Well-scoped split: each new file has a clear, single responsibility (
_types,_filters,_render,_stdin) - ✅ Solid test additions — 252 lines covering engine, staged, firewall, safe-output-type, and
buildProcessedRunedge cases - ✅
applyRunFilterslazy-parsesaw_info.jsononce and reuses it across filters — good performance hygiene preserved - ✅ Public API (
DownloadWorkflowLogs,DownloadWorkflowLogsFromStdin,LogsDownloadOptions,StdinLogsOptions) unchanged
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 135.6 AIC · ⌖ 8.27 AIC · ⊞ 6.6K
Comment /matt to run again
| pr := buildProcessedRun(result, false) | ||
| assert.Equal(t, 0, pr.Run.EffectiveTokens) | ||
| }) | ||
| } |
There was a problem hiding this comment.
[/tdd] filteredIntegrity is the only runFilterOpts field with no test coverage — a regression here is invisible.
💡 Suggested test skeleton
func TestApplyRunFilters_FilteredIntegrity(t *testing.T) {
t.Run("run without DIFC items is skipped", func(t *testing.T) {
result := makeDownloadResult(t, "")
skip := applyRunFilters(result, runFilterOpts{filteredIntegrity: true}, false)
assert.True(t, skip)
})
}Every other filter flag has a dedicated test; filteredIntegrity does not. Given that the check-error path returns true (skip) — a subtle policy choice — it should be explicitly tested.
@copilot please address this.
| StartedAt: now.Add(-5 * time.Minute), | ||
| UpdatedAt: now, | ||
| }, | ||
| LogsPath: "/tmp/test-run", |
There was a problem hiding this comment.
[/tdd] Hardcoded /tmp/test-run path makes this test fragile on systems where /tmp is not writable or differs in parallel runs.
💡 Fix
Replace with t.TempDir(), consistent with every other test case in this file:
tmpDir := t.TempDir()
result := DownloadResult{
Run: WorkflowRun{
DatabaseID: 1234,
StartedAt: now.Add(-5 * time.Minute),
UpdatedAt: now,
},
LogsPath: tmpDir,
AwContext: awCtx,
}
// assert.Equal(t, tmpDir, pr.Run.LogsPath)@copilot please address this.
| if failedJobCount, err := fetchJobStatuses(run.DatabaseID, verbose); err == nil { | ||
| run.ErrorCount += failedJobCount | ||
| if verbose && failedJobCount > 0 { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Added %d failed jobs to error count for run %d", failedJobCount, run.DatabaseID))) |
There was a problem hiding this comment.
[/codebase-design] This verbose log for failedJobCount is new for the stdin path — the old DownloadWorkflowLogsFromStdin called fetchJobStatuses silently (no matching fmt.Fprintln). The deduplicated buildProcessedRun now emits this message for both paths, changing the observable output of --stdin --verbose.
💡 Options
If the verbose log is intentionally unified, document the behaviour change in the PR description or a comment.
If the stdin path should stay silent, extract the verbose log out of buildProcessedRun and keep it only in the DownloadWorkflowLogs call site:
processedRun := buildProcessedRun(result, verbose)
// Only log failed-job count in the non-stdin path:
if verbose && processedRun.Run.ErrorCount > 0 { ... }@copilot please address this.
| }() | ||
| oldStdout := os.Stdout | ||
| defer func() { os.Stdout = oldStdout }() | ||
| os.Stdout = f |
There was a problem hiding this comment.
[/codebase-design] os.Stdout is a global; redirecting it here is not safe if any goroutine writes to stdout concurrently (or if tests ever call t.Parallel()). This pattern was carried over from the monolith — moving it into its own focused module is a good moment to flag it.
💡 Safer approach
Pass an io.Writer parameter instead of mutating the global:
func renderCrossRunReportMarkdown(report crossRunAuditReport, w io.Writer) { ... }
// call site:
renderCrossRunReportMarkdown(report, f)Or use fmt.Fprintf(f, ...) throughout renderCrossRunReportMarkdown and accept an io.Writer. This refactor isn't required now, but the new module boundary makes it an easy next step.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — clean refactor with two issues that should be addressed before merge.
### Findings summary
High — concurrency: os.Stdout = f in renderLogsOutput is a non-goroutine-safe global reassignment. This was pre-existing, but this PR funnels both DownloadWorkflowLogs and DownloadWorkflowLogsFromStdin through a single shared renderLogsOutput call site, doubling the blast radius. The right fix is to pass an io.Writer down to renderCrossRunReportMarkdown.
Medium — test correctness: One subtest in TestBuildProcessedRun hard-codes LogsPath: "/tmp/test-run" while all sibling subtests correctly use t.TempDir(). The ErrorCount == 0 assertion is only stable because fetchJobStatuses fails in CI; it would flap if run 1234 became reachable.
Overall: The deduplication is solid — applyRunFilters and buildProcessedRun are well-structured, the new tests cover the important filter combinations, and the split into focused files is a genuine improvement. Fix the two items above and this is good to go.
🔎 Code quality review by PR Code Quality Reviewer · 289.4 AIC · ⌖ 5.24 AIC · ⊞ 5.4K
Comment /review to run again
| oldStdout := os.Stdout | ||
| defer func() { os.Stdout = oldStdout }() | ||
| os.Stdout = f | ||
| renderCrossRunReportMarkdown(report) |
There was a problem hiding this comment.
os.Stdout global reassignment is not goroutine-safe — any concurrent goroutine writing to stdout during this window will have output misdirected to the report file or vice versa.
💡 Suggested fix
Pass an io.Writer to renderCrossRunReportMarkdown instead of swapping the global:
if opts.reportFile != "" {
f, err := os.Create(opts.reportFile)
if err != nil {
return fmt.Errorf("failed to create report file: %w", err)
}
defer f.Close()
renderCrossRunReportMarkdown(report, f)
} else {
renderCrossRunReportMarkdown(report, os.Stdout)
}This was pre-existing, but this refactor makes renderLogsOutput the single consolidated path for both DownloadWorkflowLogs and DownloadWorkflowLogsFromStdin. It's exactly the right moment to fix it — and the blast radius just doubled.
| func TestBuildProcessedRun(t *testing.T) { | ||
| t.Run("basic fields are propagated", func(t *testing.T) { | ||
| now := time.Now() | ||
| awCtx := &AwContext{Repo: "owner/repo"} |
There was a problem hiding this comment.
Hardcoded /tmp/test-run path makes this test non-hermetic — buildProcessedRun calls fetchJobStatuses (a live gh api network call) with run ID 1234, and ErrorCount == 0 only holds because the call fails in CI. Other subtests all use t.TempDir().
💡 Suggested fix
Replace the hardcoded path with t.TempDir() like every other subtest here:
t.Run("basic fields are propagated", func(t *testing.T) {
now := time.Now()
awCtx := &AwContext{Repo: "owner/repo"}
result := DownloadResult{
Run: WorkflowRun{
DatabaseID: 1234,
StartedAt: now.Add(-5 * time.Minute),
UpdatedAt: now,
},
LogsPath: t.TempDir(), // not a shared /tmp path
AwContext: awCtx,
}
...
})The ErrorCount == 0 assertion should also be annotated with a comment explaining it reflects only the initial reset (before fetchJobStatuses succeeds) or the test should mock the API call.
pkg/cli/logs_orchestrator.gohad grown to 1284 lines mixing orchestration, filter logic, type definitions, output rendering, and stdin processing — with ~150 lines of run-filter andProcessedRunconstruction duplicated betweenDownloadWorkflowLogsandDownloadWorkflowLogsFromStdin.Changes
logs_orchestrator_types.go(87 lines) — pure type declarations:LogsDownloadOptions,StdinLogsOptions,continuationOptions,renderLogsOutputOptionslogs_orchestrator_filters.go(184 lines) — filter logic with two new shared helpers:applyRunFilters(result, opts, verbose) bool— consolidates engine / staged / firewall / safe-output / DIFC filter blocks that were duplicated across both download pathsbuildProcessedRun(result, verbose) ProcessedRun— consolidatesProcessedRun{...}construction (duration, action minutes, effective tokens, job counts)logs_orchestrator_render.go(158 lines) —renderLogsOutput+renderLogsArtifactHintlogs_orchestrator_stdin.go(229 lines) —DownloadWorkflowLogsFromStdinlogs_orchestrator.gotrimmed from 1284 → 603 lines; retains only the pagination loop,DownloadWorkflowLogs, and small utilitieslogs_orchestrator_filters_test.go— unit tests forapplyRunFiltersandbuildProcessedRunExample: deduplicated filter call
Before (identical block existed in both
DownloadWorkflowLogsandDownloadWorkflowLogsFromStdin, ~80 lines each):After (one call, both paths):
Public API (
DownloadWorkflowLogs,DownloadWorkflowLogsFromStdin,LogsDownloadOptions,StdinLogsOptions) is unchanged.