Refactor DownloadWorkflowLogs to LogsDownloadOptions and migrate all callsites

docs/adr/31336-refactor-download-workflow-logs-to-options-struct.md

@@ -0,0 +1,79 @@
+# ADR-31336: Refactor `DownloadWorkflowLogs` to a Typed Options Struct
+
+**Date**: 2026-05-10
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+`DownloadWorkflowLogs` in `pkg/cli/logs_orchestrator.go` had grown to a 26-parameter positional signature, including several adjacent `bool` parameters (e.g., `verbose, toolGraph, noStaged, firewallOnly, noFirewall, parse, jsonOutput, ...`). Eight callsites — one production callsite in `pkg/cli/logs_command.go` and seven test callsites — passed arguments by position, making them brittle and hard to read. Adjacent same-typed parameters carry a real risk of silent mis-ordering that the compiler cannot catch, and any addition or reordering of parameters required mechanical churn at every callsite. The function's orchestration logic is otherwise intact and not the subject of this change.
+
+### Decision
+
+We will replace the positional parameter list of `DownloadWorkflowLogs` with a typed options struct, `LogsDownloadOptions`, and update the signature to `func DownloadWorkflowLogs(ctx context.Context, opts LogsDownloadOptions) error`. All existing callers will pass a named struct literal, allowing each call to specify only the fields it cares about while zero-valued fields preserve prior defaults. This is an interface refactor only — the orchestration logic, behavior, and field semantics are unchanged.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep the positional signature
+
+Leaving the 26-parameter signature in place avoids any code churn but preserves the legibility and correctness risks (silent mis-ordering between adjacent bools, churn at every callsite when a parameter is added). Given that callsites already required line-by-line `// fieldName` comments to remain readable, this was rejected as continuing to accumulate technical debt.
+
+#### Alternative 2: Functional options pattern (`WithX(...)` constructors)
+
+Using a chain of `Option` functions (`opts ...Option`) is idiomatic in some Go libraries and offers extensibility for defaults. It was rejected because the parameter set is a flat configuration bag with no need for hidden state, lazy evaluation, or extensibility hooks; a plain struct literal is more discoverable in IDEs, simpler to read, and aligns with how callers (CLI flag plumbing) already aggregate values.
+
+#### Alternative 3: Split the function into smaller orchestrators
+
+Decomposing `DownloadWorkflowLogs` into multiple smaller functions, each with fewer parameters, would address the symptom by reducing surface area. It was rejected for this PR because it is a logic rewrite rather than an interface refactor and would expand scope significantly; the options-struct change is a low-risk first step that does not preclude future decomposition.
+
+### Consequences
+
+#### Positive
+- Callsites become self-documenting via field names; mis-ordering between same-typed parameters is no longer possible.
+- Adding a new optional parameter is a non-breaking change for callers that don't use it (zero value preserves prior behavior).
+- Test code becomes substantially shorter — only fields relevant to the test need to be set.
+
+#### Negative
+- The shim inside `DownloadWorkflowLogs` that re-binds every option field to a local variable preserves the existing function body verbatim but adds 25 trivial lines; future cleanup should reference `opts.X` directly and remove the shim.
+- The public API of `pkg/cli` changes shape, so any out-of-tree caller (if any exist) must migrate.
+- A 26-field struct is itself a smell; the struct documents the shape of the problem but does not solve it. Decomposition (Alternative 3) is still warranted as follow-up.
+
+#### Neutral
+- Field defaults are now expressed implicitly via Go zero values rather than explicit positional arguments; reviewers must verify that "omitted" matches "previously passed as zero."
+- All known callers (1 production + 7 tests) have been migrated in this PR; no deprecation period is needed.
+
+---
+
+## Part 2 — Normative Specification (RFC 2119)
+
+> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
+
+### Function Signature
+
+1. The exported function `DownloadWorkflowLogs` **MUST** have the signature `func DownloadWorkflowLogs(ctx context.Context, opts LogsDownloadOptions) error`.
+2. The exported type `LogsDownloadOptions` **MUST** be a struct defined in `pkg/cli/logs_orchestrator.go`.
+3. New configuration parameters added to the workflow log download flow **MUST** be added as fields on `LogsDownloadOptions` rather than as additional positional parameters.
+4. `DownloadWorkflowLogs` **MUST NOT** reintroduce any positional configuration parameters beyond `ctx` and `opts`.
+
+### Caller Conventions
+
+1. Callers of `DownloadWorkflowLogs` **MUST** pass a `LogsDownloadOptions` value constructed as a named struct literal (i.e., `LogsDownloadOptions{Field: value, ...}`).
+2. Callers **SHOULD** specify only the fields whose non-zero values are meaningful for the call; reliance on zero values to express defaults is **RECOMMENDED**.
+3. Test code **SHOULD NOT** restate the full set of fields when a subset suffices.
+
+### Field Semantics
+
+1. Zero values for fields on `LogsDownloadOptions` **MUST** preserve the same behavior as the previous positional signature passed `0`, `""`, `false`, or `nil` for the corresponding argument.
+2. Changes to the meaning of any existing field on `LogsDownloadOptions` **MUST** be recorded in a new ADR that supersedes or amends this one.
+
+### Conformance
+
+An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/25633125088) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/cli/context_cancellation_test.go

@@ -71,7 +71,10 @@ func TestDownloadWorkflowLogsWithCancellation(t *testing.T) {
 	cancel()
 
 	// Try to download logs with a cancelled context
-	err := DownloadWorkflowLogs(ctx, "", 10, "", "", "/tmp/test-logs", "", "", 0, 0, "", false, false, false, false, false, false, false, 0, "", "", false, false, "", nil, "")
+	err := DownloadWorkflowLogs(ctx, LogsDownloadOptions{
+		Count:     10,
+		OutputDir: "/tmp/test-logs",
+	})
 
 	// Should return context.Canceled error
 	assert.ErrorIs(t, err, context.Canceled, "Should return context.Canceled error when context is cancelled")
@@ -104,14 +107,19 @@ func TestRunWorkflowsOnGitHubCancellationDuringExecution(t *testing.T) {
 	assert.Error(t, err, "Should return an error")
 }
 
-// TestDownloadWorkflowLogsTimeoutRespected tests that timeout is converted to context deadline
+// TestDownloadWorkflowLogsTimeoutRespected tests that timeout-minutes is respected
 func TestDownloadWorkflowLogsTimeoutRespected(t *testing.T) {
-	// Test with a very short timeout (1 second) and verify the function returns quickly
+	// Use a short timeout in minutes and verify fast-fail behavior still returns quickly
 	ctx := context.Background()
 
 	start := time.Now()
 	// Use a workflow name that doesn't exist to avoid actual network calls
-	_ = DownloadWorkflowLogs(ctx, "nonexistent-workflow-12345", 100, "", "", "/tmp/test-logs", "", "", 0, 0, "", false, false, false, false, false, false, false, 1, "", "", false, false, "", nil, "")
+	_ = DownloadWorkflowLogs(ctx, LogsDownloadOptions{
+		WorkflowName:   "nonexistent-workflow-12345",
+		Count:          100,
+		OutputDir:      "/tmp/test-logs",
+		TimeoutMinutes: 1,
+	})
 	elapsed := time.Since(start)
 
 	// Should complete within reasonable time (give 5 seconds buffer for test overhead)

pkg/cli/logs_ci_scenario_test.go

@@ -29,34 +29,15 @@ func TestLogsJSONOutputWithNoRuns(t *testing.T) {
 
 	// Call DownloadWorkflowLogs with parameters that will result in no matching runs
 	// We use a non-existent workflow name to ensure no results
-	err := DownloadWorkflowLogs(
-		ctx,
-		"nonexistent-workflow-12345", // Workflow that doesn't exist
-		2,                            // count
-		"",                           // startDate
-		"",                           // endDate
-		tmpDir,                       // outputDir
-		"copilot",                    // engine
-		"",                           // ref
-		0,                            // beforeRunID
-		0,                            // afterRunID
-		"",                           // repoOverride
-		false,                        // verbose
-		false,                        // toolGraph
-		false,                        // noStaged
-		false,                        // firewallOnly
-		false,                        // noFirewall
-		false,                        // parse
-		true,                         // jsonOutput - THIS IS KEY
-		10,                           // timeout
-		"summary.json",               // summaryFile
-		"",                           // safeOutputType
-		false,                        // filteredIntegrity
-		false,                        // train
-		"",                           // format
-		nil,                          // artifactSets
-		"",                           // after
-	)
+	err := DownloadWorkflowLogs(ctx, LogsDownloadOptions{
+		WorkflowName:   "nonexistent-workflow-12345", // Workflow that doesn't exist
+		Count:          2,
+		OutputDir:      tmpDir,
+		Engine:         "copilot",
+		JSONOutput:     true, // THIS IS KEY
+		TimeoutMinutes: 10,
+		SummaryFile:    "summary.json",
+	})
 
 	// Restore stdout and read output
 	w.Close()

pkg/cli/logs_command.go

@@ -243,7 +243,33 @@ Examples:
 
 			logsCommandLog.Printf("Executing logs download: workflow=%s, count=%d, engine=%s, train=%v, after=%s", workflowName, count, engine, train, after)
 
-			return DownloadWorkflowLogs(cmd.Context(), workflowName, count, startDate, endDate, outputDir, engine, ref, beforeRunID, afterRunID, repoOverride, verbose, toolGraph, noStaged, firewallOnly, noFirewall, parse, jsonOutput, timeout, summaryFile, safeOutputType, filteredIntegrity, train, format, artifacts, after)
+			return DownloadWorkflowLogs(cmd.Context(), LogsDownloadOptions{
+				WorkflowName:      workflowName,
+				Count:             count,
+				StartDate:         startDate,
+				EndDate:           endDate,
+				OutputDir:         outputDir,
+				Engine:            engine,
+				Ref:               ref,
+				BeforeRunID:       beforeRunID,
+				AfterRunID:        afterRunID,
+				RepoOverride:      repoOverride,
+				Verbose:           verbose,
+				ToolGraph:         toolGraph,
+				NoStaged:          noStaged,
+				FirewallOnly:      firewallOnly,
+				NoFirewall:        noFirewall,
+				Parse:             parse,
+				JSONOutput:        jsonOutput,
+				TimeoutMinutes:    timeout,
+				SummaryFile:       summaryFile,
+				SafeOutputType:    safeOutputType,
+				FilteredIntegrity: filteredIntegrity,
+				Train:             train,
+				Format:            format,
+				ArtifactSets:      artifacts,
+				After:             after,
+			})
 		},
 	}
 

pkg/cli/logs_download_test.go

@@ -21,7 +21,11 @@ func TestDownloadWorkflowLogs(t *testing.T) {
 	// Test the DownloadWorkflowLogs function
 	// This should either fail with auth error (if not authenticated)
 	// or succeed with no results (if authenticated but no workflows match)
-	err := DownloadWorkflowLogs(context.Background(), "", 1, "", "", "./test-logs", "", "", 0, 0, "", false, false, false, false, false, false, false, 0, "summary.json", "", false, false, "", nil, "")
+	err := DownloadWorkflowLogs(context.Background(), LogsDownloadOptions{
+		Count:       1,
+		OutputDir:   "./test-logs",
+		SummaryFile: "summary.json",
+	})
 
 	// If GitHub CLI is authenticated, the function may succeed but find no results
 	// If not authenticated, it should return an auth error
@@ -393,7 +397,12 @@ func TestDownloadWorkflowLogsWithEngineFilter(t *testing.T) {
 			if !tt.expectError {
 				// For valid engines, test that the function can be called without panic
 				// It may still fail with auth errors, which is expected
-				err := DownloadWorkflowLogs(context.Background(), "", 1, "", "", "./test-logs", tt.engine, "", 0, 0, "", false, false, false, false, false, false, false, 0, "summary.json", "", false, false, "", nil, "")
+				err := DownloadWorkflowLogs(context.Background(), LogsDownloadOptions{
+					Count:       1,
+					OutputDir:   "./test-logs",
+					Engine:      tt.engine,
+					SummaryFile: "summary.json",
+				})
 
 				// Clean up any created directories
 				os.RemoveAll("./test-logs")

pkg/cli/logs_json_stderr_order_test.go

@@ -36,34 +36,15 @@ func TestLogsJSONOutputBeforeStderr(t *testing.T) {
 
 	// Call DownloadWorkflowLogs with parameters that will result in no matching runs
 	// This should trigger the warning message path
-	err := DownloadWorkflowLogs(
-		ctx,
-		"nonexistent-workflow-test-12345", // Workflow that doesn't exist
-		2,                                 // count
-		"",                                // startDate
-		"",                                // endDate
-		tmpDir,                            // outputDir
-		"copilot",                         // engine
-		"",                                // ref
-		0,                                 // beforeRunID
-		0,                                 // afterRunID
-		"",                                // repoOverride
-		false,                             // verbose
-		false,                             // toolGraph
-		false,                             // noStaged
-		false,                             // firewallOnly
-		false,                             // noFirewall
-		false,                             // parse
-		true,                              // jsonOutput - THIS IS KEY
-		10,                                // timeout
-		"summary.json",                    // summaryFile
-		"",                                // safeOutputType
-		false,                             // filteredIntegrity
-		false,                             // train
-		"",                                // format
-		nil,                               // artifactSets
-		"",                                // after
-	)
+	err := DownloadWorkflowLogs(ctx, LogsDownloadOptions{
+		WorkflowName:   "nonexistent-workflow-test-12345", // Workflow that doesn't exist
+		Count:          2,
+		OutputDir:      tmpDir,
+		Engine:         "copilot",
+		JSONOutput:     true, // THIS IS KEY
+		TimeoutMinutes: 10,
+		SummaryFile:    "summary.json",
+	})
 
 	// Close writers first
 	stdoutW.Close()
@@ -162,34 +143,15 @@ func TestLogsJSONAndStderrRedirected(t *testing.T) {
 
 	// Call DownloadWorkflowLogs
 	ctx := context.Background()
-	err := DownloadWorkflowLogs(
-		ctx,
-		"nonexistent-workflow-ci-test-67890",
-		2,
-		"",
-		"",
-		tmpDir,
-		"copilot",
-		"",
-		0,
-		0,
-		"",
-		false,
-		false,
-		false,
-		false,
-		false,
-		false,
-		true, // jsonOutput
-		10,
-		"summary.json",
-		"",    // safeOutputType
-		false, // filteredIntegrity
-		false, // train
-		"",    // format
-		nil,   // artifactSets
-		"",    // after
-	)
+	err := DownloadWorkflowLogs(ctx, LogsDownloadOptions{
+		WorkflowName:   "nonexistent-workflow-ci-test-67890",
+		Count:          2,
+		OutputDir:      tmpDir,
+		Engine:         "copilot",
+		JSONOutput:     true,
+		TimeoutMinutes: 10,
+		SummaryFile:    "summary.json",
+	})
 
 	// Close the writer
 	w.Close()