Refactor safe-outputs config parsing into focused workflow modules

docs/adr/44313-split-safe-outputs-config-into-focused-modules.md

@@ -0,0 +1,56 @@
+# ADR-44313: Split Safe-Outputs Config Parsing into Focused Modules
+
+**Date**: 2026-07-09
+**Status**: Draft
+**Deciders**: pelikhan, copilot-swe-agent
+
+---
+
+### Context
+
+`pkg/workflow/safe_outputs_config.go` had grown to 1,254 lines and contained a single `extractSafeOutputsConfig` function of ~760 lines that mixed configuration type definitions, frontmatter extraction orchestration, per-handler parsing helpers, bounded-integer parsing, and handler-manager config assembly. This made it difficult to navigate, understand the boundaries between concerns, and safely extend individual pieces in isolation. Bounded-integer parsing logic for `max-patch-size`, `max-patch-files`, and `timeout-minutes` was duplicated across three separate blocks with subtle inconsistencies between them.
+
+### Decision
+
+We will split `safe_outputs_config.go` into six focused files within the same `workflow` package, each owning a single concern:
+- `safe_outputs_config_types.go` — shared config types and logger
+- `safe_outputs_config_extraction.go` — top-level frontmatter extraction orchestration
+- `safe_outputs_config_global.go` — global safe-outputs field parsing
+- `safe_outputs_config_base.go` — shared per-handler base parsing helpers (`parseBaseSafeOutputConfig`, `parseSamplesValue`)
+- `safe_outputs_config_runtime.go` — handler-manager config assembly and serialization
+- Original `safe_outputs_config.go` — deleted
+
+We will also extract a shared `parseBoundedIntField` / `parseBoundedIntFieldOrDefault` helper and reuse it for all three bounded-integer fields, eliminating the duplicated type-switch blocks.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep the monolithic file with improved internal documentation
+
+Add section headers and inline comments to make the single file easier to navigate without changing the file structure.
+
+This was not chosen because it addresses only discoverability, not the underlying readability and maintainability problems. A reader still needs to hold the entire 1,254-line file in context, and adding shared helpers for bounded-integer parsing still requires the same refactor within the file. A doc-only fix would also not enforce module boundaries or make future additions land in the right place.
+
+#### Alternative 2: Move each concern into a separate sub-package under `pkg/workflow/safeoutputs/`
+
+Create a dedicated sub-package (`pkg/workflow/safeoutputs/`) with exported types and functions, giving each file full package-level separation.
+
+This was not chosen because it would require exporting currently unexported symbols (types, helpers, the logger), changing call sites across the `workflow` package, and adding a circular-import risk. The benefit of true package isolation did not outweigh the cost of the broader refactor and the API surface change. Staying in the same package achieves the readability goal with minimal blast radius.
+
+### Consequences
+
+#### Positive
+- Each file is focused on a single responsibility, making the codebase easier to navigate and understand
+- Shared `parseBoundedIntField` / `parseBoundedIntFieldOrDefault` eliminates three copies of the type-switch parsing logic, reducing the risk of inconsistent handling for future bounded-integer fields
+- New test coverage for the shared bounded-int helper documents and locks in truncation, clamping, and invalid-value behavior
+
+#### Negative
+- More files to open when tracing a full code path through config extraction (six files instead of one)
+- The split is behavior-preserving by intent, but the refactor introduces risk that subtle behavioral differences could creep in if the split was not perfectly faithful to the original; test coverage and reviewer attention are required to catch these
+
+#### Neutral
+- All changes are internal to the `workflow` package; no public API surface or exported symbols change
+- The `extractSafeOutputsConfig` function remains the single entry point for callers, so external call sites are unaffected
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/workflow/safe_outputs_config_base.go

@@ -0,0 +1,107 @@
+package workflow
+
+import (
+	"strings"
+
+	"github.com/github/gh-aw/pkg/typeutil"
+)
+
+// parseBaseSafeOutputConfig parses common fields (max, github-token, github-app, staged) from a config map.
+// If defaultMax is provided (> 0), it will be set as the default value for config.Max
+// before parsing the max field from configMap. Supports both integer values and GitHub
+// Actions expression strings (e.g. "${{ inputs.max }}").
+func (c *Compiler) parseBaseSafeOutputConfig(configMap map[string]any, config *BaseSafeOutputConfig, defaultMax int) {
+	// Set default max if provided
+	if defaultMax > 0 {
+		safeOutputsConfigLog.Printf("Setting default max: %d", defaultMax)
+		config.Max = defaultIntStr(defaultMax)
+	}
+
+	// Parse max (this will override the default if present in configMap)
+	if max, exists := configMap["max"]; exists {
+		switch v := max.(type) {
+		case string:
+			// Accept GitHub Actions expression strings
+			if strings.HasPrefix(v, "${{") && strings.HasSuffix(v, "}}") {
+				safeOutputsConfigLog.Printf("Parsed max as GitHub Actions expression: %s", v)
+				config.Max = &v
+			}
+		default:
+			// Convert integer/float64/etc to string via typeutil.ParseIntValue
+			if maxInt, ok := typeutil.ParseIntValue(max); ok {
+				safeOutputsConfigLog.Printf("Parsed max as integer: %d", maxInt)
+				s := defaultIntStr(maxInt)
+				config.Max = s
+			}
+		}
+	}
+
+	// Parse github-token
+	if githubToken, exists := configMap["github-token"]; exists {
+		if githubTokenStr, ok := githubToken.(string); ok {
+			safeOutputsConfigLog.Print("Parsed custom github-token from config")
+			config.GitHubToken = githubTokenStr
+		}
+	}
+
+	// Parse github-app (per-handler GitHub App credentials for token minting)
+	if app, exists := configMap["github-app"]; exists {
+		if appMap, ok := app.(map[string]any); ok {
+			safeOutputsConfigLog.Print("Parsed custom github-app from config")
+			config.GitHubApp = parseAppConfig(appMap)
+		}
+	}
+
+	// Parse staged flag (per-handler staged mode)
+	if err := preprocessBoolFieldAsString(configMap, "staged", safeOutputsConfigLog); err != nil {
+		safeOutputsConfigLog.Printf("Invalid staged value: %v", err)
+	} else if staged, exists := configMap["staged"]; exists {
+		if stagedStr, ok := staged.(string); ok && stagedStr != "" {
+			safeOutputsConfigLog.Printf("Parsed staged flag: %s", stagedStr)
+			value := TemplatableBool(stagedStr)
+			config.Staged = &value
+		}
+	}
+
+	// Parse samples list (hidden feature: deterministic replay samples for --use-samples).
+	// Accepts either a YAML list of objects, or a single object that is auto-wrapped
+	// into a one-element list. The JSON schema rejects scalar/string shapes so we
+	// don't need a defensive YAML-string branch here.
+	if samples, exists := configMap["samples"]; exists {
+		parsed := parseSamplesValue(samples)
+		if len(parsed) > 0 {
+			safeOutputsConfigLog.Printf("Parsed %d samples entries", len(parsed))
+			config.Samples = parsed
+		}
+	}
+}
+
+// parseSamplesValue normalizes a `samples` frontmatter value into a list of
+// objects. Accepted shapes:
+//   - YAML list of mappings: returned as-is
+//   - single YAML mapping: wrapped into a one-element list
+//
+// Any other shape returns an empty slice — schema validation rejects those
+// shapes upstream and we keep this parser strict to match.
+func parseSamplesValue(samples any) []map[string]any {
+	switch v := samples.(type) {
+	case []any:
+		out := make([]map[string]any, 0, len(v))
+		for _, item := range v {
+			if m, ok := item.(map[string]any); ok {
+				out = append(out, m)
+			} else if mStr, ok := item.(map[string]string); ok {
+				converted := make(map[string]any, len(mStr))
+				for k, s := range mStr {
+					converted[k] = s
+				}
+				out = append(out, converted)
+			}
+		}
+		return out
+	case map[string]any:
+		return []map[string]any{v}
+	default:
+		return nil
+	}
+}