refactor: split logs_orchestrator.go (1284 lines) into 5 focused modules

docs/adr/44051-split-logs-orchestrator-focused-modules.md

@@ -0,0 +1,45 @@
+# ADR-44051: Split logs_orchestrator.go into Focused Modules
+
+**Date**: 2026-07-07
+**Status**: Draft
+**Deciders**: Unknown (generated from PR #44051 diff)
+
+---
+
+### Context
+
+`pkg/cli/logs_orchestrator.go` had grown to 1284 lines, mixing orchestration, filter logic, type definitions, output rendering, and stdin processing in a single file. Approximately 150 lines of run-filter logic and `ProcessedRun` construction were duplicated verbatim between `DownloadWorkflowLogs` and `DownloadWorkflowLogsFromStdin`. Any bug fix or behavioral change to the filter pipeline had to be applied in two places, creating an active maintenance risk. The codebase uses the `pkg/cli` package for all command-line surface area; Go does not require splitting a package across multiple files, but readability and testability suffer when a single file grows past ~300–400 lines.
+
+### Decision
+
+We will split `logs_orchestrator.go` into five focused files within the same `cli` package: `logs_orchestrator_types.go` (option structs and internal types), `logs_orchestrator_filters.go` (filter helpers `applyRunFilters` and `buildProcessedRun`), `logs_orchestrator_render.go` (output rendering), `logs_orchestrator_stdin.go` (`DownloadWorkflowLogsFromStdin`), and a trimmed `logs_orchestrator.go` retaining only the pagination loop and `DownloadWorkflowLogs`. The public API (`DownloadWorkflowLogs`, `DownloadWorkflowLogsFromStdin`, `LogsDownloadOptions`, `StdinLogsOptions`) is unchanged. Unit tests for the newly extracted helpers are added in `logs_orchestrator_filters_test.go`.
+
+### Alternatives Considered
+
+#### Alternative 1: Deduplicate helpers without file splitting
+
+Introduce `applyRunFilters` and `buildProcessedRun` as private helpers inside `logs_orchestrator.go` without moving any code to new files. This eliminates the duplication and is the minimal change, but the file would remain ~900 lines with type definitions, rendering, stdin processing, and the core pagination loop still entangled. Navigation and isolated testing of each concern would still be difficult.
+
+#### Alternative 2: Move each concern into a separate Go package
+
+Create sub-packages such as `pkg/cli/logsfilters` and `pkg/cli/logsrender`. This enforces hard encapsulation boundaries at the package level and prevents accidental coupling. However, it requires exporting all shared types (e.g., `DownloadResult`, `ProcessedRun`) that are currently package-private, widens the public API surface, and increases the cost of future changes that cross the new package boundaries. Given the single-package nature of the CLI layer, the encapsulation benefit does not justify the added complexity.
+
+### Consequences
+
+#### Positive
+- Eliminates ~150 lines of duplicated filter and `ProcessedRun` construction code, so future filter changes are applied once and tested once.
+- Smaller, single-concern files (87–252 lines each) are faster to navigate, review, and modify independently.
+- The new `applyRunFilters` and `buildProcessedRun` helpers are directly unit-testable without exercising the full download pipeline.
+- No callers outside the package need to change; the public API is stable.
+
+#### Negative
+- Tracing the complete logic for a single download path (e.g., `DownloadWorkflowLogs`) now requires opening multiple files rather than scrolling within one.
+- If the filter semantics for the stdin path and the standard path ever need to diverge significantly, the shared `applyRunFilters` function becomes a constraint that must be refactored.
+
+#### Neutral
+- All five files remain in `package cli`; there is no package boundary or import-cycle risk introduced.
+- The `logs_orchestrator_filters_test.go` file uses `package cli` (not `package cli_test`), so it retains access to package-private identifiers needed to construct test fixtures.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/audit_cross_run_render.go

@@ -3,6 +3,7 @@ package cli
 import (
 	"encoding/json"
 	"fmt"
+	"io"
 	"os"
 	"strconv"
 	"strings"
@@ -25,157 +26,165 @@ func renderCrossRunReportJSON(report *CrossRunAuditReport) error {
 
 // renderCrossRunReportMarkdown outputs the cross-run report as Markdown to stdout.
 func renderCrossRunReportMarkdown(report *CrossRunAuditReport) {
+	renderCrossRunReportMarkdownToWriter(os.Stdout, report)
+}
+
+func renderCrossRunReportMarkdownToWriter(w io.Writer, report *CrossRunAuditReport) {
 	crossRunRenderLog.Printf("Rendering cross-run report as markdown: runs_analyzed=%d, domains=%d", report.RunsAnalyzed, len(report.DomainInventory))
-	fmt.Fprintln(os.Stdout, "# Audit Report — Cross-Run Analysis")
-	fmt.Fprintln(os.Stdout)
-
-	renderMarkdownExecutiveSummary(report)
-	renderMarkdownMetricsTrend(report.MetricsTrend)
-	renderMarkdownMCPHealth(report)
-	renderMarkdownErrorTrend(report)
-	renderMarkdownDomainInventory(report)
-	renderMarkdownDrain3Insights(report.Drain3Insights)
-	renderMarkdownPerRunBreakdown(report.PerRunBreakdown)
+	fmt.Fprintln(w, "# Audit Report — Cross-Run Analysis")
+	fmt.Fprintln(w)
+
+	renderMarkdownExecutiveSummaryToWriter(w, report)
+	renderMarkdownMetricsTrendToWriter(w, report.MetricsTrend)
+	renderMarkdownMCPHealthToWriter(w, report)
+	renderMarkdownErrorTrendToWriter(w, report)
+	renderMarkdownDomainInventoryToWriter(w, report)
+	renderMarkdownDrain3InsightsToWriter(w, report.Drain3Insights)
+	renderMarkdownPerRunBreakdownToWriter(w, report.PerRunBreakdown)
 }
 
-func renderMarkdownExecutiveSummary(report *CrossRunAuditReport) {
-	fmt.Fprintln(os.Stdout, "## Executive Summary")
-	fmt.Fprintln(os.Stdout)
-	fmt.Fprintf(os.Stdout, "| Metric | Value |\n")
-	fmt.Fprintf(os.Stdout, "|--------|-------|\n")
-	fmt.Fprintf(os.Stdout, "| Runs analyzed | %d |\n", report.RunsAnalyzed)
-	fmt.Fprintf(os.Stdout, "| Runs with firewall data | %d |\n", report.RunsWithData)
-	fmt.Fprintf(os.Stdout, "| Runs without firewall data | %d |\n", report.RunsWithoutData)
-	fmt.Fprintf(os.Stdout, "| Total requests | %d |\n", report.Summary.TotalRequests)
-	fmt.Fprintf(os.Stdout, "| Allowed requests | %d |\n", report.Summary.TotalAllowed)
-	fmt.Fprintf(os.Stdout, "| Blocked requests | %d |\n", report.Summary.TotalBlocked)
-	fmt.Fprintf(os.Stdout, "| Overall denial rate | %.1f%% |\n", report.Summary.OverallDenyRate*100)
-	fmt.Fprintf(os.Stdout, "| Unique domains | %d |\n", report.Summary.UniqueDomains)
-	fmt.Fprintln(os.Stdout)
+func renderMarkdownExecutiveSummaryToWriter(w io.Writer, report *CrossRunAuditReport) {
+	fmt.Fprintln(w, "## Executive Summary")
+	fmt.Fprintln(w)
+	fmt.Fprintf(w, "| Metric | Value |\n")
+	fmt.Fprintf(w, "|--------|-------|\n")
+	fmt.Fprintf(w, "| Runs analyzed | %d |\n", report.RunsAnalyzed)
+	fmt.Fprintf(w, "| Runs with firewall data | %d |\n", report.RunsWithData)
+	fmt.Fprintf(w, "| Runs without firewall data | %d |\n", report.RunsWithoutData)
+	fmt.Fprintf(w, "| Total requests | %d |\n", report.Summary.TotalRequests)
+	fmt.Fprintf(w, "| Allowed requests | %d |\n", report.Summary.TotalAllowed)
+	fmt.Fprintf(w, "| Blocked requests | %d |\n", report.Summary.TotalBlocked)
+	fmt.Fprintf(w, "| Overall denial rate | %.1f%% |\n", report.Summary.OverallDenyRate*100)
+	fmt.Fprintf(w, "| Unique domains | %d |\n", report.Summary.UniqueDomains)
+	fmt.Fprintln(w)
 }
 
 func renderMarkdownMetricsTrend(mt MetricsTrendData) {
+	renderMarkdownMetricsTrendToWriter(os.Stdout, mt)
+}
+
+func renderMarkdownMetricsTrendToWriter(w io.Writer, mt MetricsTrendData) {
 	if mt.TotalTokens == 0 && mt.TotalTurns == 0 && mt.AvgDurationNs == 0 {
 		return
 	}
 
-	fmt.Fprintln(os.Stdout, "## Metrics Trends")
-	fmt.Fprintln(os.Stdout)
-	fmt.Fprintf(os.Stdout, "| Metric | Total | Avg/run | Min | Max | Spikes |\n")
-	fmt.Fprintf(os.Stdout, "|--------|-------|---------|-----|-----|--------|\n")
+	fmt.Fprintln(w, "## Metrics Trends")
+	fmt.Fprintln(w)
+	fmt.Fprintf(w, "| Metric | Total | Avg/run | Min | Max | Spikes |\n")
+	fmt.Fprintf(w, "|--------|-------|---------|-----|-----|--------|\n")
 	if mt.TotalTokens > 0 {
 		spikes := "—"
 		if len(mt.TokenSpikes) > 0 {
 			spikes = "⚠ " + formatRunIDs(mt.TokenSpikes)
 		}
-		fmt.Fprintf(os.Stdout, "| Token Trend | %d | %d | %d | %d | %s |\n",
+		fmt.Fprintf(w, "| Token Trend | %d | %d | %d | %d | %s |\n",
 			mt.TotalTokens, mt.AvgTokens, mt.MinTokens, mt.MaxTokens, spikes)
 	}
 	if mt.TotalTurns > 0 {
-		fmt.Fprintf(os.Stdout, "| Turns | %d | %.1f | — | %d | — |\n",
+		fmt.Fprintf(w, "| Turns | %d | %.1f | — | %d | — |\n",
 			mt.TotalTurns, mt.AvgTurns, mt.MaxTurns)
 	}
 	if mt.AvgDurationNs > 0 {
-		fmt.Fprintf(os.Stdout, "| Duration | — | %s | %s | %s | — |\n",
+		fmt.Fprintf(w, "| Duration | — | %s | %s | %s | — |\n",
 			timeutil.FormatDurationNs(mt.AvgDurationNs),
 			timeutil.FormatDurationNs(mt.MinDurationNs),
 			timeutil.FormatDurationNs(mt.MaxDurationNs))
 	}
-	fmt.Fprintln(os.Stdout)
+	fmt.Fprintln(w)
 }
 
-func renderMarkdownMCPHealth(report *CrossRunAuditReport) {
+func renderMarkdownMCPHealthToWriter(w io.Writer, report *CrossRunAuditReport) {
 	if len(report.MCPHealth) == 0 {
 		return
 	}
-	fmt.Fprintf(os.Stdout, "## MCP Server Health (%d runs)\n\n", report.RunsAnalyzed)
-	fmt.Fprintf(os.Stdout, "| Server | Connected | Error Rate | Total Calls | Errors | Status |\n")
-	fmt.Fprintf(os.Stdout, "|--------|-----------|------------|-------------|--------|--------|\n")
+	fmt.Fprintf(w, "## MCP Server Health (%d runs)\n\n", report.RunsAnalyzed)
+	fmt.Fprintf(w, "| Server | Connected | Error Rate | Total Calls | Errors | Status |\n")
+	fmt.Fprintf(w, "|--------|-----------|------------|-------------|--------|--------|\n")
 	for _, h := range report.MCPHealth {
 		status := "✅ ok"
 		if h.Unreliable {
 			status = "⚠ unreliable"
 		}
-		fmt.Fprintf(os.Stdout, "| `%s` | %d/%d | %.1f%% | %d | %d | %s |\n",
+		fmt.Fprintf(w, "| `%s` | %d/%d | %.1f%% | %d | %d | %s |\n",
 			h.ServerName, h.RunsConnected, h.TotalRuns,
 			h.ErrorRate*100, h.TotalCalls, h.TotalErrors, status)
 	}
-	fmt.Fprintln(os.Stdout)
+	fmt.Fprintln(w)
 }
 
-func renderMarkdownErrorTrend(report *CrossRunAuditReport) {
+func renderMarkdownErrorTrendToWriter(w io.Writer, report *CrossRunAuditReport) {
 	et := report.ErrorTrend
 	if et.TotalErrors == 0 && et.TotalWarnings == 0 {
 		return
 	}
-	fmt.Fprintln(os.Stdout, "## Error Trend")
-	fmt.Fprintln(os.Stdout)
-	fmt.Fprintf(os.Stdout, "| Metric | Value |\n")
-	fmt.Fprintf(os.Stdout, "|--------|-------|\n")
-	fmt.Fprintf(os.Stdout, "| Runs with errors | %d/%d (%.0f%%) |\n",
+	fmt.Fprintln(w, "## Error Trend")
+	fmt.Fprintln(w)
+	fmt.Fprintf(w, "| Metric | Value |\n")
+	fmt.Fprintf(w, "|--------|-------|\n")
+	fmt.Fprintf(w, "| Runs with errors | %d/%d (%.0f%%) |\n",
 		et.RunsWithErrors, report.RunsAnalyzed,
 		safePercent(et.RunsWithErrors, report.RunsAnalyzed))
-	fmt.Fprintf(os.Stdout, "| Total errors | %d |\n", et.TotalErrors)
-	fmt.Fprintf(os.Stdout, "| Avg errors/run | %.2f |\n", et.AvgErrorsPerRun)
+	fmt.Fprintf(w, "| Total errors | %d |\n", et.TotalErrors)
+	fmt.Fprintf(w, "| Avg errors/run | %.2f |\n", et.AvgErrorsPerRun)
 	if et.TotalWarnings > 0 {
-		fmt.Fprintf(os.Stdout, "| Runs with warnings | %d/%d |\n", et.RunsWithWarnings, report.RunsAnalyzed)
-		fmt.Fprintf(os.Stdout, "| Total warnings | %d |\n", et.TotalWarnings)
+		fmt.Fprintf(w, "| Runs with warnings | %d/%d |\n", et.RunsWithWarnings, report.RunsAnalyzed)
+		fmt.Fprintf(w, "| Total warnings | %d |\n", et.TotalWarnings)
 	}
-	fmt.Fprintln(os.Stdout)
+	fmt.Fprintln(w)
 }
 
-func renderMarkdownDomainInventory(report *CrossRunAuditReport) {
+func renderMarkdownDomainInventoryToWriter(w io.Writer, report *CrossRunAuditReport) {
 	if len(report.DomainInventory) == 0 {
 		return
 	}
-	fmt.Fprintln(os.Stdout, "## Domain Inventory")
-	fmt.Fprintln(os.Stdout)
-	fmt.Fprintf(os.Stdout, "| Domain | Status | Seen In | Allowed | Blocked |\n")
-	fmt.Fprintf(os.Stdout, "|--------|--------|---------|---------|--------|\n")
+	fmt.Fprintln(w, "## Domain Inventory")
+	fmt.Fprintln(w)
+	fmt.Fprintf(w, "| Domain | Status | Seen In | Allowed | Blocked |\n")
+	fmt.Fprintf(w, "|--------|--------|---------|---------|--------|\n")
 	for _, entry := range report.DomainInventory {
-		fmt.Fprintf(os.Stdout, "| `%s` | %s %s | %d/%d runs | %d | %d |\n",
+		fmt.Fprintf(w, "| `%s` | %s %s | %d/%d runs | %d | %d |\n",
 			entry.Domain, firewallStatusEmoji(entry.OverallStatus), entry.OverallStatus,
 			entry.SeenInRuns, report.RunsAnalyzed, entry.TotalAllowed, entry.TotalBlocked)
 	}
-	fmt.Fprintln(os.Stdout)
+	fmt.Fprintln(w)
 }
 
-func renderMarkdownDrain3Insights(insights []ObservabilityInsight) {
+func renderMarkdownDrain3InsightsToWriter(w io.Writer, insights []ObservabilityInsight) {
 	if len(insights) == 0 {
 		return
 	}
 	crossRunRenderLog.Printf("Rendering markdown drain3 insights: count=%d", len(insights))
-	fmt.Fprintln(os.Stdout, "## Agent Event Pattern Analysis")
-	fmt.Fprintln(os.Stdout)
-	fmt.Fprintf(os.Stdout, "| Severity | Category | Title | Summary |\n")
-	fmt.Fprintf(os.Stdout, "|----------|----------|-------|--------|\n")
+	fmt.Fprintln(w, "## Agent Event Pattern Analysis")
+	fmt.Fprintln(w)
+	fmt.Fprintf(w, "| Severity | Category | Title | Summary |\n")
+	fmt.Fprintf(w, "|----------|----------|-------|--------|\n")
 	for _, insight := range insights {
 		summary := insight.Summary
 		if insight.Evidence != "" {
 			summary += " (" + insight.Evidence + ")"
 		}
-		fmt.Fprintf(os.Stdout, "| %s %s | %s | %s | %s |\n",
+		fmt.Fprintf(w, "| %s %s | %s | %s | %s |\n",
 			renderSeverityIcon(insight.Severity), insight.Severity, insight.Category, insight.Title, summary)
 	}
-	fmt.Fprintln(os.Stdout)
+	fmt.Fprintln(w)
 }
 
-func renderMarkdownPerRunBreakdown(runs []PerRunFirewallBreakdown) {
+func renderMarkdownPerRunBreakdownToWriter(w io.Writer, runs []PerRunFirewallBreakdown) {
 	if len(runs) == 0 {
 		return
 	}
-	fmt.Fprintln(os.Stdout, "## Per-Run Breakdown")
-	fmt.Fprintln(os.Stdout)
-	fmt.Fprintf(os.Stdout, "| Run ID | Workflow | Conclusion | Duration | Firewall | Tokens | Turns | MCP Err | Errors |\n")
-	fmt.Fprintf(os.Stdout, "|--------|----------|------------|----------|----------|--------|-------|---------|--------|\n")
+	fmt.Fprintln(w, "## Per-Run Breakdown")
+	fmt.Fprintln(w)
+	fmt.Fprintf(w, "| Run ID | Workflow | Conclusion | Duration | Firewall | Tokens | Turns | MCP Err | Errors |\n")
+	fmt.Fprintf(w, "|--------|----------|------------|----------|----------|--------|-------|---------|--------|\n")
 	for _, run := range runs {
 		firewallCol, tokenStr, turnsStr, durStr := markdownPerRunFields(run)
-		fmt.Fprintf(os.Stdout, "| %d | %s | %s | %s | %s | %s | %s | %d | %d |\n",
+		fmt.Fprintf(w, "| %d | %s | %s | %s | %s | %s | %s | %d | %d |\n",
 			run.RunID, run.WorkflowName, run.Conclusion, durStr,
 			firewallCol, tokenStr, turnsStr,
 			run.MCPErrors, run.ErrorCount)
 	}
-	fmt.Fprintln(os.Stdout)
+	fmt.Fprintln(w)
 }
 
 func markdownPerRunFields(run PerRunFirewallBreakdown) (string, string, string, string) {