Refactor interactive/audit CLI large functions into focused helpers

docs/adr/49799-decompose-cli-largefunc-helpers.md

@@ -0,0 +1,44 @@
+# ADR-49799: Decompose Oversized CLI Functions into Focused Helper Functions
+
+**Date**: 2026-08-02
+**Status**: Accepted
+**Deciders**: pelikhan, app/copilot-swe-agent
+
+---
+
+### Context
+
+The codebase enforces a custom `largefunc` linter rule capping functions at 60 lines (`make golangci-lint`). Several CLI entry-point and flow-control functions in `pkg/cli/` exceeded this limit: `RunAddInteractive` (126 lines), `createWorkflowPRAndConfigureSecret` (171 lines), `computeFirewallDiff` (144 lines), `parseSquidAccessLog` (80 lines), `buildAction` (83 lines), and `selectAIEngineAndKey` (118 lines). These functions mixed multiple sequential phases — host auto-detection, preflight checks, PR merge orchestration, secret configuration, and domain diff classification — into single units, making them difficult to test in isolation and harder to reason about under review.
+
+### Decision
+
+We will decompose each oversized CLI function into focused, single-responsibility helper functions co-located in the same file and package. Each extracted helper covers exactly one logical phase (e.g., host detection, PR merge loop, domain diff entry construction) and is tested independently where behavior is non-trivial. The `mergeAction` type and its constants, previously function-local, are promoted to package scope to allow multiple helpers to reference them without re-declaration.
+
+### Alternatives Considered
+
+#### Alternative 1: Raise the `largefunc` lint threshold
+
+Increase the per-function line limit (or add per-file lint suppressions) to accommodate the current oversized functions without refactoring. This avoids code churn but leaves the underlying mixed-responsibility problem in place, degrades long-term readability, and weakens the lint rule for the rest of the codebase.
+
+#### Alternative 2: Extract logic into separate packages or interface types
+
+Move the orchestration phases into new dedicated packages or types rather than inline helper functions in the same file. This would provide module-level separation of concerns but would introduce new packages, potential import complexity, and substantial structural churn for what is fundamentally a sequential orchestration flow with no reuse across packages.
+
+### Consequences
+
+#### Positive
+- All affected functions pass the 60-line `largefunc` lint check, unblocking CI.
+- Extracted helpers are individually testable; focused unit tests were added for `prioritizeEngineOption` and `buildMergeOptions`.
+- Orchestrator functions (`RunAddInteractive`, `createWorkflowPRAndConfigureSecret`) now read as a sequence of high-level named steps, improving code review clarity.
+
+#### Negative
+- Some extracted helpers carry high parameter counts (e.g., `processSquidAccessLogLine` takes five parameters) because context is passed down rather than captured in a receiver or struct — trading function-length compliance for increased parameter coupling.
+- The call graph is deeper; understanding the full flow now requires tracing through more helper function boundaries.
+
+#### Neutral
+- Command behavior and semantics are fully preserved; existing tests continue to pass unmodified.
+- The `mergeAction` type and constants are now package-level rather than closure-local; this widens their scope but does not expose them outside the package.
+
+---
+
+*ADR created by [adr-writer agent]. Status: Accepted.*

pkg/cli/access_log.go

@@ -103,54 +103,7 @@ func parseSquidAccessLog(logPath string, verbose bool) (*DomainAnalysis, error)
 
 	scanner := bufio.NewScanner(file)
 	for scanner.Scan() {
-		line := strings.TrimSpace(scanner.Text())
-		if line == "" || strings.HasPrefix(line, "#") {
-			continue
-		}
-
-		entry, err := parseSquidLogLine(line)
-		if err != nil {
-			if verbose {
-				fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to parse log line: %v", err)))
-			}
-			continue
-		}
-
-		analysis.TotalRequests++
-
-		// Extract domain from URL
-		domain := stringutil.ExtractDomainFromURL(entry.URL)
-		if domain == "" {
-			continue
-		}
-
-		// Determine if request was allowed or blocked based on status code
-		// Squid typically returns:
-		// - 200, 206, 304: Allowed/successful
-		// - 403: Forbidden (blocked by ACL)
-		// - 407: Proxy authentication required
-		// - 502, 503: Connection/upstream errors
-		statusCode := entry.Status
-		isAllowed := statusCode == "TCP_HIT/200" || statusCode == "TCP_MISS/200" ||
-			statusCode == "TCP_REFRESH_MODIFIED/200" || statusCode == "TCP_IMS_HIT/304" ||
-			strings.Contains(statusCode, "/200") || strings.Contains(statusCode, "/206") ||
-			strings.Contains(statusCode, "/304")
-
-		if isAllowed {
-			analysis.AllowedRequests++
-			if !setutil.Contains(allowedDomainsSet, domain) {
-				allowedDomainsSet[domain] = struct {
-				}{}
-				analysis.AllowedDomains = append(analysis.AllowedDomains, domain)
-			}
-		} else {
-			analysis.BlockedRequests++
-			if !setutil.Contains(blockedDomainsSet, domain) {
-				blockedDomainsSet[domain] = struct {
-				}{}
-				analysis.BlockedDomains = append(analysis.BlockedDomains, domain)
-			}
-		}
+		processSquidAccessLogLine(strings.TrimSpace(scanner.Text()), verbose, analysis, allowedDomainsSet, blockedDomainsSet)
 	}
 
 	if err := scanner.Err(); err != nil {
@@ -167,6 +120,50 @@ func parseSquidAccessLog(logPath string, verbose bool) (*DomainAnalysis, error)
 	return analysis, nil
 }
 
+func processSquidAccessLogLine(line string, verbose bool, analysis *DomainAnalysis, allowedDomainsSet, blockedDomainsSet map[string]struct{}) {
+	if line == "" || strings.HasPrefix(line, "#") {
+		return
+	}
+
+	entry, err := parseSquidLogLine(line)
+	if err != nil {
+		if verbose {
+			fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to parse log line: %v", err)))
+		}
+		return
+	}
+
+	analysis.TotalRequests++
+	domain := stringutil.ExtractDomainFromURL(entry.URL)
+	if domain == "" {
+		return
+	}
+
+	if isAllowedSquidStatus(entry.Status) {
+		analysis.AllowedRequests++
+		addUniqueDomain(allowedDomainsSet, domain, &analysis.AllowedDomains)
+		return
+	}
+
+	analysis.BlockedRequests++
+	addUniqueDomain(blockedDomainsSet, domain, &analysis.BlockedDomains)
+}
+
+func isAllowedSquidStatus(statusCode string) bool {
+	return statusCode == "TCP_HIT/200" || statusCode == "TCP_MISS/200" ||
+		statusCode == "TCP_REFRESH_MODIFIED/200" || statusCode == "TCP_IMS_HIT/304" ||
+		strings.Contains(statusCode, "/200") || strings.Contains(statusCode, "/206") ||
+		strings.Contains(statusCode, "/304")
+}
+
+func addUniqueDomain(domainSet map[string]struct{}, domain string, domains *[]string) {
+	if setutil.Contains(domainSet, domain) {
+		return
+	}
+	domainSet[domain] = struct{}{}
+	*domains = append(*domains, domain)
+}
+
 // parseSquidLogLine parses a single squid access log line
 // Squid log format: timestamp duration client status size method url user hierarchy type
 func parseSquidLogLine(line string) (*AccessLogEntry, error) {

pkg/cli/actions_build_command.go

@@ -228,6 +228,14 @@ func buildAction(actionsDir, actionName string) error {
 		return nil
 	}
 
+	if err := buildJavaScriptAction(actionPath, actionName); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func buildJavaScriptAction(actionPath, actionName string) error {
 	srcPath := filepath.Join(actionPath, "src", "index.js")
 	outputPath := filepath.Join(actionPath, "index.js")
 

pkg/cli/add_interactive_engine.go

@@ -24,56 +24,8 @@ func (c *AddInteractiveConfig) selectAIEngineAndKey() error {
 		return err
 	}
 
-	// Determine default engine based on existing secrets, workflow preference, then environment
-	// Priority order: flag override > existing secrets > workflow frontmatter > environment > default
-	defaultEngine := string(constants.DefaultEngine)
-	workflowSpecifiedEngine := ""
-
-	// Check if workflow specifies a preferred engine in frontmatter
-	if c.resolvedWorkflows != nil && len(c.resolvedWorkflows.Workflows) > 0 {
-		for _, wf := range c.resolvedWorkflows.Workflows {
-			if wf.Engine != "" {
-				workflowSpecifiedEngine = wf.Engine
-				addInteractiveLog.Printf("Workflow specifies engine in frontmatter: %s", wf.Engine)
-				break
-			}
-		}
-	}
-
-	// If engine is explicitly overridden via flag, use that
-	if c.EngineOverride != "" {
-		defaultEngine = c.EngineOverride
-	} else {
-		// Priority 1: Check existing repository secrets using EngineOptions
-		// This takes precedence over workflow preference since users should use what's already available
-		for _, opt := range constants.EngineOptions {
-			if setutil.Contains(c.existingSecrets, opt.SecretName) {
-				defaultEngine = opt.Value
-				addInteractiveLog.Printf("Found existing secret %s, recommending engine: %s", opt.SecretName, opt.Value)
-				break
-			}
-		}
-
-		// Priority 2: If no existing secret found, use workflow frontmatter preference
-		if defaultEngine == string(constants.DefaultEngine) && workflowSpecifiedEngine != "" {
-			defaultEngine = workflowSpecifiedEngine
-		}
-
-		// Priority 3: Check environment variables if no existing secret or workflow preference found
-		if defaultEngine == string(constants.DefaultEngine) && workflowSpecifiedEngine == "" {
-			for _, opt := range constants.EngineOptions {
-				envVar := opt.SecretName
-				if opt.EnvVarName != "" {
-					envVar = opt.EnvVarName
-				}
-				if lookupEnv(envVar) != "" {
-					defaultEngine = opt.Value
-					addInteractiveLog.Printf("Found env var %s, recommending engine: %s", envVar, opt.Value)
-					break
-				}
-			}
-		}
-	}
+	workflowSpecifiedEngine := c.getWorkflowSpecifiedEngine()
+	defaultEngine := c.determineDefaultEngine(workflowSpecifiedEngine)
 
 	// If engine is already overridden, skip selection
 	if c.EngineOverride != "" {
@@ -88,35 +40,11 @@ func (c *AddInteractiveConfig) selectAIEngineAndKey() error {
 
 	// Build engine options with notes about existing secrets and workflow specification.
 	// The list of engines is derived from the catalog to ensure all registered engines appear.
-	catalog := workflow.NewEngineCatalog(workflow.NewEngineRegistry())
-	engineOptions := sliceutil.Map(catalog.All(), func(def *workflow.EngineDefinition) huh.Option[string] {
-		opt := constants.GetEngineOption(def.ID)
-		label := fmt.Sprintf("%s - %s", def.DisplayName, def.Description)
-		// Add markers for secret availability and workflow specification.
-		// opt may be nil for catalog engines not yet represented in EngineOptions;
-		// in that case we conservatively show '[no secret]'.
-		if opt != nil && setutil.Contains(c.existingSecrets, opt.SecretName) {
-			label += " [secret exists]"
-		} else {
-			label += " [no secret]"
-		}
-		if def.ID == workflowSpecifiedEngine {
-			label += " [specified in workflow]"
-		}
-		return huh.NewOption(label, def.ID)
-	})
+	engineOptions := c.buildEngineOptions(workflowSpecifiedEngine)
 
 	var selectedEngine string
 
-	// Set the default selection by moving it to front
-	for i, opt := range engineOptions {
-		if opt.Value == defaultEngine {
-			if i > 0 {
-				engineOptions[0], engineOptions[i] = engineOptions[i], engineOptions[0]
-			}
-			break
-		}
-	}
+	prioritizeEngineOption(engineOptions, defaultEngine)
 
 	fmt.Fprintln(os.Stderr, "")
 	form := console.NewSelectForm(
@@ -137,6 +65,86 @@ func (c *AddInteractiveConfig) selectAIEngineAndKey() error {
 	return c.configureEngineAPISecret(selectedEngine)
 }
 
+func (c *AddInteractiveConfig) getWorkflowSpecifiedEngine() string {
+	if c.resolvedWorkflows == nil || len(c.resolvedWorkflows.Workflows) == 0 {
+		return ""
+	}
+
+	for _, wf := range c.resolvedWorkflows.Workflows {
+		if wf.Engine == "" {
+			continue
+		}
+		addInteractiveLog.Printf("Workflow specifies engine in frontmatter: %s", wf.Engine)
+		return wf.Engine
+	}
+	return ""
+}
+
+func (c *AddInteractiveConfig) determineDefaultEngine(workflowSpecifiedEngine string) string {
+	defaultEngine := string(constants.DefaultEngine)
+	if c.EngineOverride != "" {
+		return c.EngineOverride
+	}
+
+	for _, opt := range constants.EngineOptions {
+		if setutil.Contains(c.existingSecrets, opt.SecretName) {
+			addInteractiveLog.Printf("Found existing secret %s, recommending engine: %s", opt.SecretName, opt.Value)
+			if opt.Value != string(constants.DefaultEngine) {
+				return opt.Value
+			}
+			// The secret maps to the default engine; fall through so that a
+			// workflow-specified engine or env-var credential can still override it.
+			break
+		}
+	}
+
+	if workflowSpecifiedEngine != "" {
+		return workflowSpecifiedEngine
+	}
+
+	for _, opt := range constants.EngineOptions {
+		envVar := opt.SecretName
+		if opt.EnvVarName != "" {
+			envVar = opt.EnvVarName
+		}
+		if lookupEnv(envVar) != "" {
+			addInteractiveLog.Printf("Found env var %s, recommending engine: %s", envVar, opt.Value)
+			return opt.Value
+		}
+	}
+
+	return defaultEngine
+}
+
+func (c *AddInteractiveConfig) buildEngineOptions(workflowSpecifiedEngine string) []huh.Option[string] {
+	catalog := workflow.NewEngineCatalog(workflow.NewEngineRegistry())
+	return sliceutil.Map(catalog.All(), func(def *workflow.EngineDefinition) huh.Option[string] {
+		opt := constants.GetEngineOption(def.ID)
+		label := fmt.Sprintf("%s - %s", def.DisplayName, def.Description)
+		if opt != nil && setutil.Contains(c.existingSecrets, opt.SecretName) {
+			label += " [secret exists]"
+		} else {
+			label += " [no secret]"
+		}
+		if def.ID == workflowSpecifiedEngine {
+			label += " [specified in workflow]"
+		}
+		return huh.NewOption(label, def.ID)
+	})
+}
+
+func prioritizeEngineOption(engineOptions []huh.Option[string], defaultEngine string) {
+	for i, opt := range engineOptions {
+		if opt.Value != defaultEngine {
+			continue
+		}
+		if i > 0 {
+			engineOptions[0], engineOptions[i] = engineOptions[i], engineOptions[0]
+		}
+		return
+	}
+}
+
 // configureEngineAPISecret collects the API key for the selected engine using the unified engine secrets functions
 func (c *AddInteractiveConfig) configureEngineAPISecret(engine string) error {
 	addInteractiveLog.Printf("Collecting API key for engine: %s", engine)