Refactor: `DownloadWorkflowLogs` has 26 positional parameters — introduce options struct

[github-actions]

Sergo finding (Run 5, 2026-05-10)

Function-complexity scan flagged DownloadWorkflowLogs for both length (508 lines) and an extreme positional parameter list (#aw_sg5fn2).

Location

• File: pkg/cli/logs_orchestrator.go
• Symbol: DownloadWorkflowLogs
• Body location (Serena LSP): lines 38–546 (508 lines)
• Signature (line 39):

func DownloadWorkflowLogs(
    ctx context.Context, workflowName string, count int,
    startDate, endDate, outputDir, engine, ref string,
    beforeRunID, afterRunID int64,
    repoOverride string, verbose bool, toolGraph bool, noStaged bool,
    firewallOnly bool, noFirewall bool, parse bool, jsonOutput bool,
    timeout int, summaryFile string, safeOutputType string,
    filteredIntegrity bool, train bool, format string,
    artifactSets []string, after string,
) error

That is 26 positional parameters (1 ctx + 9 strings + 5 ints/int64 + 9 bools + 1 []string + 1 string).

Why it matters

serena find_referencing_symbols shows 1 production caller (NewLogsCommand in pkg/cli/logs_command.go:245) and 7 test callers. Every test must spell out all 26 args by position, e.g.:

// pkg/cli/context_cancellation_test.go:73
err := DownloadWorkflowLogs(ctx, "", 10, "", "", "/tmp/test-logs", "", "", 0, 0, "",
    false, false, false, false, false, false, false, 0, "", "", false, false, "", nil, "")

This is brittle for two reasons:

1. Adding/removing/reordering any flag is a 9-call edit even though only one site (NewLogsCommand) actually populates non-zero values. Recent git history shows new fields appended several times — train, format, artifactSets, after are clearly later additions.
2. Test calls are unreadable — a reader cannot tell from false, false, false, false, false, false, false, 0 which flag is which without counting columns against the signature.

Severity

Medium — no bug today, but each subsequent flag adds churn to 8 call sites and every flag is a meaning-by-position trap (swapping two adjacent bools would compile cleanly and silently change behaviour).

Recommendation

Introduce a LogsDownloadOptions struct and reduce the public function to (ctx, opts) error. Group the parameters by purpose:

type LogsDownloadOptions struct {
    // selection
    WorkflowName string
    Count        int
    StartDate    string
    EndDate      string
    Engine       string
    Ref          string
    BeforeRunID  int64
    AfterRunID   int64
    RepoOverride string

    // output
    OutputDir       string
    SummaryFile     string
    SafeOutputType  string
    Format          string
    JSONOutput      bool

    // download behaviour
    Timeout       int  // minutes
    ArtifactSets  []string
    After         string
    NoStaged      bool

    // log processing
    Parse           bool
    ToolGraph       bool
    FirewallOnly    bool
    NoFirewall      bool
    FilteredIntegrity bool
    Train           bool

    // diagnostics
    Verbose bool
}

func DownloadWorkflowLogs(ctx context.Context, opts LogsDownloadOptions) error { ... }

Callsite becomes:

// production callsite (logs_command.go:245)
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,
    Timeout: timeout, SummaryFile: summaryFile,
    SafeOutputType: safeOutputType, FilteredIntegrity: filteredIntegrity,
    Train: train, Format: format,
    ArtifactSets: artifacts, After: after,
})

And tests can name only the fields they actually exercise:

// pkg/cli/context_cancellation_test.go (after refactor)
err := DownloadWorkflowLogs(ctx, LogsDownloadOptions{ Count: 10, OutputDir: "/tmp/test-logs" })

Validation

• Migrate the 1 production caller and 7 test callers (Serena confirms exactly 8 sites)
• Run go test ./pkg/cli/... — all 7 test callers exercise the function
• Field-by-field check that grouping above doesn't change defaults (every existing false/""/0 becomes the zero value of the corresponding struct field — semantically identical)
• Optional: keep a thin compatibility wrapper if external callers exist (verify there are none with a fresh serena find_referencing_symbols)

Estimated effort

Small (mechanical refactor, no logic changes inside the body — only the parameter list and the 8 callsites change).

---

Generated by Sergo — Strategy: function-complexity-and-long-function-hot-spots
Run: §25620196889

Generated by Sergo - Serena Go Expert · ● 77.2M · ◷

• expires on May 17, 2026, 5:21 AM UTC