Consolidate duplicate runs-on normalization helpers across config parsers

docs/adr/52975-consolidate-runs-on-normalization-helpers.md

@@ -0,0 +1,55 @@
+# ADR-52975: Consolidate runs-on Normalization Helpers into runs_on_snippet.go
+
+**Date**: 2026-08-15
+**Status**: Draft
+**Deciders**: pelikhan (via copilot-swe-agent, PR #52975)
+
+---
+
+### Context
+
+The `pkg/workflow` package contained three separate locations that independently implemented overlapping `runs-on` parsing and rendering logic:
+
+- `repo_config.go` defined `RunsOnValue`, `UnmarshalJSON`, `toRunsOnValue`, `isRunsOnArrayValue`, and `FormatRunsOn`
+- `safe_jobs.go` contained inline branching logic for array-vs-scalar `runs-on` rendering
+- `runs_on_snippet.go` already held `renderRunsOnSnippet`/`normalizeRunsOnSnippet` helpers
+
+This duplication meant that a bug fix to runner-shape handling could be applied in one parser while silently missing the others. The `runs-on` field accepts both a single string and an array of strings (e.g., for self-hosted runners with labels), and the normalization and YAML rendering must behave consistently across all callers.
+
+### Decision
+
+We will consolidate all `runs-on` type definitions and helpers into `runs_on_snippet.go`, which already houses the core snippet rendering logic. Specifically:
+
+- `RunsOnValue`, `UnmarshalJSON`, `toRunsOnValue`, `isRunsOnArrayValue`, and `FormatRunsOn` are moved from `repo_config.go` into `runs_on_snippet.go`.
+- A new `formatSafeJobRunsOn` helper is added to `runs_on_snippet.go`, replacing the inline array-vs-scalar branching in `safe_jobs.go`'s job-building loop.
+- `safe_jobs.go` is simplified to a single call to `formatSafeJobRunsOn`.
+
+This is a pure code-organization refactor with no change to supported `runs-on` shapes or rendered output.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep Duplication As-Is
+
+Leave `RunsOnValue` and its helpers in `repo_config.go` and retain the inline branching in `safe_jobs.go`. Simple in the short term and zero risk of behavioral regression, but perpetuates the maintenance hazard: the next `runs-on` bug fix must be applied in multiple places, and there is no structural enforcement ensuring all parsers stay in sync.
+
+#### Alternative 2: Create a Dedicated `runs_on.go` File
+
+Move all `runs-on` types and helpers into a new `pkg/workflow/runs_on.go` file rather than expanding `runs_on_snippet.go`. This would produce a cleaner name-to-responsibility mapping. The trade-off is an additional file that must be discovered, and `runs_on_snippet.go`'s snippet rendering helpers would remain separated from the type that drives them. Given the functions are tightly coupled (they all operate on `RunsOnValue` and produce YAML fragments), co-location in one file is preferred over splitting across two.
+
+### Consequences
+
+#### Positive
+- Single source of truth for all `runs-on` normalization and YAML rendering; future bug fixes or new runner-shape support apply uniformly across `aw.json` and `safe-outputs.jobs` parsing.
+- `safe_jobs.go` call site is reduced from 13 lines of branching logic to 1 line, improving readability and reducing cognitive overhead for future maintainers.
+
+#### Negative
+- `runs_on_snippet.go` now covers a broader scope than its filename implies (it holds the `RunsOnValue` type, JSON unmarshaling, and YAML rendering helpers, not just snippet generation). Readers may be surprised to find the type definition there rather than in a file named `runs_on.go`.
+- As a pure refactor, behavioral parity must be verified by existing tests. Any gap in test coverage of `runs-on` edge cases (empty arrays, single empty-string elements, multi-label arrays) could mask an unintended regression introduced during the move.
+
+#### Neutral
+- The `encoding/json` and `fmt` imports are added to `runs_on_snippet.go` (previously only in `repo_config.go`) as a direct consequence of moving the type and its JSON unmarshaler.
+- No public API surface changes: `RunsOnValue`, `FormatRunsOn`, and related helpers remain exported at the same package level.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/workflow/repo_config.go

@@ -62,68 +62,6 @@ const RepoConfigFileName = ".github/workflows/aw.json"
 // for action failure issues created by the conclusion job.
 const DefaultActionFailureIssueExpiresHours = 24 * 7
 
-// RunsOnValue is a JSON-deserializable type for the runs_on field in aw.json.
-// It accepts either a single runner label string or an array of runner label strings.
-// When unmarshalled, a plain string is normalised to a single-element slice so the
-// rest of the code works with a uniform []string type.
-type RunsOnValue []string
-
-// UnmarshalJSON implements json.Unmarshaler, accepting either a JSON string or
-// a JSON array of strings for the runs_on field.
-func (r *RunsOnValue) UnmarshalJSON(data []byte) error {
-	// Try plain string first (runs_on: "ubuntu-latest")
-	var s string
-	if err := json.Unmarshal(data, &s); err == nil {
-		*r = RunsOnValue{s}
-		return nil
-	}
-
-	// Try array of strings (runs_on: ["self-hosted", "linux"])
-	var ss []string
-	if err := json.Unmarshal(data, &ss); err != nil {
-		return fmt.Errorf("runs_on value is not recognized: %w. Expected a string or array of strings, for example: runs_on: \"ubuntu-latest\"", err)
-	}
-	*r = RunsOnValue(ss)
-	return nil
-}
-
-// toRunsOnValue converts a YAML-decoded runs-on value (a string or a list of
-// strings) into a RunsOnValue. Values with an unsupported shape, and non-string
-// list entries, are ignored.
-func toRunsOnValue(value any) RunsOnValue {
-	switch v := value.(type) {
-	case string:
-		return RunsOnValue{v}
-	case []any:
-		labels := make(RunsOnValue, 0, len(v))
-		for _, item := range v {
-			if itemStr, ok := item.(string); ok {
-				labels = append(labels, itemStr)
-			}
-		}
-		if len(labels) == 0 {
-			return nil
-		}
-		return labels
-	case []string:
-		if len(v) == 0 {
-			return nil
-		}
-		return RunsOnValue(v)
-	default:
-		return nil
-	}
-}
-
-func isRunsOnArrayValue(value any) bool {
-	switch value.(type) {
-	case []any, []string:
-		return true
-	default:
-		return false
-	}
-}
-
 // MaintenanceConfig holds maintenance-workflow-specific settings from aw.json.
 type MaintenanceCompileConfig struct {
 	// CreatePullRequestGitHubToken is the secret name used by the compile-workflows
@@ -459,37 +397,6 @@ func validateRepoConfigValues(cfg *RepoConfig) error {
 	return nil
 }
 
-// FormatRunsOn serialises a RunsOnValue to a YAML-compatible string that can
-// be inlined directly after "runs-on: " in a generated workflow.
-//
-//   - empty / nil  → defaultRunsOn is returned
-//   - single label → the label string (e.g. "ubuntu-latest")
-//   - multiple labels → JSON-encoded flow sequence, e.g. ["self-hosted","linux"]
-//
-// For multi-label values json.Marshal is used so that any characters that are
-// special in YAML or JSON (quotes, backslashes, …) are properly escaped.
-// The schema already forbids newlines and control characters, providing a
-// defence-in-depth against YAML injection.
-func FormatRunsOn(runsOn RunsOnValue, defaultRunsOn string) string {
-	if len(runsOn) == 0 {
-		return defaultRunsOn
-	}
-	if len(runsOn) == 1 {
-		if runsOn[0] == "" {
-			return defaultRunsOn
-		}
-		return runsOn[0]
-	}
-	// Multiple labels: use json.Marshal to produce a properly-escaped YAML
-	// flow sequence.  A JSON array is valid YAML flow sequence notation.
-	encoded, err := json.Marshal([]string(runsOn))
-	if err != nil {
-		// []string marshalling never fails; fall back to the default just in case.
-		return defaultRunsOn
-	}
-	return string(encoded)
-}
-
 // ActionFailureIssueExpiresHours returns the configured action failure issue
 // expiration in hours, or the default value when unset.
 func (r *RepoConfig) ActionFailureIssueExpiresHours() int {

pkg/workflow/runs_on_snippet.go

@@ -1,6 +1,8 @@
 package workflow
 
 import (
+	"encoding/json"
+	"fmt"
 	"strings"
 
 	"github.com/github/gh-aw/pkg/logger"
@@ -9,6 +11,117 @@ import (
 
 var runsOnSnippetLog = logger.New("workflow:runs_on_snippet")
 
+// RunsOnValue is a JSON-deserializable type for runs_on/runs-on fields that
+// accept either a single runner label string or an array of runner label
+// strings (e.g. aw.json's maintenance.runs_on and safe-outputs.jobs runs-on).
+// When unmarshalled, a plain string is normalised to a single-element slice so
+// the rest of the code works with a uniform []string type.
+type RunsOnValue []string
+
+// UnmarshalJSON implements json.Unmarshaler, accepting either a JSON string or
+// a JSON array of strings for the runs_on field.
+func (r *RunsOnValue) UnmarshalJSON(data []byte) error {
+	// Try plain string first (runs_on: "ubuntu-latest")
+	var s string
+	if err := json.Unmarshal(data, &s); err == nil {
+		*r = RunsOnValue{s}
+		return nil
+	}
+
+	// Try array of strings (runs_on: ["self-hosted", "linux"])
+	var ss []string
+	if err := json.Unmarshal(data, &ss); err != nil {
+		return fmt.Errorf("runs_on value is not recognized: %w. Expected a string or array of strings, for example: runs_on: \"ubuntu-latest\"", err)
+	}
+	*r = RunsOnValue(ss)
+	return nil
+}
+
+// toRunsOnValue converts a YAML-decoded runs-on value (a string or a list of
+// strings) into a RunsOnValue. Values with an unsupported shape, and non-string
+// list entries, are ignored.
+func toRunsOnValue(value any) RunsOnValue {
+	switch v := value.(type) {
+	case string:
+		return RunsOnValue{v}
+	case []any:
+		labels := make(RunsOnValue, 0, len(v))
+		for _, item := range v {
+			if itemStr, ok := item.(string); ok {
+				labels = append(labels, itemStr)
+			}
+		}
+		if len(labels) == 0 {
+			return nil
+		}
+		return labels
+	case []string:
+		if len(v) == 0 {
+			return nil
+		}
+		return RunsOnValue(v)
+	default:
+		return nil
+	}
+}
+
+// isRunsOnArrayValue reports whether a YAML-decoded runs-on value has array
+// shape (as opposed to a single string label).
+func isRunsOnArrayValue(value any) bool {
+	switch value.(type) {
+	case []any, []string:
+		return true
+	default:
+		return false
+	}
+}
+
+// FormatRunsOn serialises a RunsOnValue to a YAML-compatible string that can
+// be inlined directly after "runs-on: " in a generated workflow.
+//
+//   - empty / nil  → defaultRunsOn is returned
+//   - single label → the label string (e.g. "ubuntu-latest")
+//   - multiple labels → JSON-encoded flow sequence, e.g. ["self-hosted","linux"]
+//
+// For multi-label values json.Marshal is used so that any characters that are
+// special in YAML or JSON (quotes, backslashes, …) are properly escaped.
+// The schema already forbids newlines and control characters, providing a
+// defence-in-depth against YAML injection.
+func FormatRunsOn(runsOn RunsOnValue, defaultRunsOn string) string {
+	if len(runsOn) == 0 {
+		return defaultRunsOn
+	}
+	if len(runsOn) == 1 {
+		if runsOn[0] == "" {
+			return defaultRunsOn
+		}
+		return runsOn[0]
+	}
+	// Multiple labels: use json.Marshal to produce a properly-escaped YAML
+	// flow sequence.  A JSON array is valid YAML flow sequence notation.
+	encoded, err := json.Marshal([]string(runsOn))
+	if err != nil {
+		// []string marshalling never fails; fall back to the default just in case.
+		return defaultRunsOn
+	}
+	return string(encoded)
+}
+
+// formatSafeJobRunsOn renders the runs-on YAML fragment (including the
+// leading "runs-on:" key) for a safe-job, given its parsed RunsOnValue and
+// whether the original configured value had array shape. This centralizes the
+// array-vs-scalar rendering decision so callers don't need to special-case it.
+func formatSafeJobRunsOn(runsOn RunsOnValue, runsOnArray bool, defaultRunsOn string) string {
+	// Keep []string{""} semantically unset, matching FormatRunsOn behavior.
+	if runsOnArray && len(runsOn) > 0 && (len(runsOn) != 1 || runsOn[0] != "") {
+		if snippet := renderRunsOnSnippet([]string(runsOn)); snippet != "" {
+			return snippet
+		}
+	}
+	// FormatRunsOn handles defaulting and YAML-safe rendering for scalar values.
+	return "runs-on: " + FormatRunsOn(runsOn, defaultRunsOn)
+}
+
 func runsOnMarshalOptions() []yaml.EncodeOption {
 	opts := append([]yaml.EncodeOption{}, DefaultMarshalOptions...)
 	return append(opts, yaml.IndentSequence(true))

pkg/workflow/runs_on_snippet_test.go

@@ -0,0 +1,72 @@
+package workflow
+
+import "testing"
+
+func TestFormatSafeJobRunsOn(t *testing.T) {
+	tests := []struct {
+		name          string
+		runsOn        RunsOnValue
+		runsOnArray   bool
+		defaultRunsOn string
+		want          string
+	}{
+		{
+			name:          "nil value defaults",
+			runsOn:        nil,
+			runsOnArray:   false,
+			defaultRunsOn: "ubuntu-latest",
+			want:          "runs-on: ubuntu-latest",
+		},
+		{
+			name:          "empty array-shaped value defaults",
+			runsOn:        RunsOnValue{},
+			runsOnArray:   true,
+			defaultRunsOn: "ubuntu-latest",
+			want:          "runs-on: ubuntu-latest",
+		},
+		{
+			name:          "single empty-string element treated as unset",
+			runsOn:        RunsOnValue{""},
+			runsOnArray:   true,
+			defaultRunsOn: "ubuntu-latest",
+			want:          "runs-on: ubuntu-latest",
+		},
+		{
+			name:          "scalar value rendered inline",
+			runsOn:        RunsOnValue{"self-hosted"},
+			runsOnArray:   false,
+			defaultRunsOn: "ubuntu-latest",
+			want:          "runs-on: self-hosted",
+		},
+		{
+			name:          "single-element array shape renders as YAML sequence",
+			runsOn:        RunsOnValue{"self-hosted"},
+			runsOnArray:   true,
+			defaultRunsOn: "ubuntu-latest",
+			want:          "runs-on:\n  - self-hosted",
+		},
+		{
+			name:          "multi-element array shape renders YAML sequence",
+			runsOn:        RunsOnValue{"self-hosted", "linux"},
+			runsOnArray:   true,
+			defaultRunsOn: "ubuntu-latest",
+			want:          "runs-on:\n  - self-hosted\n  - linux",
+		},
+		{
+			name:          "multi-element scalar-mode renders as JSON array inline",
+			runsOn:        RunsOnValue{"self-hosted", "linux"},
+			runsOnArray:   false,
+			defaultRunsOn: "ubuntu-latest",
+			want:          `runs-on: ["self-hosted","linux"]`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := formatSafeJobRunsOn(tt.runsOn, tt.runsOnArray, tt.defaultRunsOn)
+			if got != tt.want {
+				t.Errorf("formatSafeJobRunsOn(%#v, %v, %q) = %q, want %q", tt.runsOn, tt.runsOnArray, tt.defaultRunsOn, got, tt.want)
+			}
+		})
+	}
+}

pkg/workflow/safe_jobs.go

@@ -236,19 +236,10 @@ func (c *Compiler) buildSafeJobs(data *WorkflowData, threatDetectionEnabled bool
 
 		const defaultRunsOn = "ubuntu-latest"
 
-		// Set runs-on.
-		// Preserve list-shaped input from safe-outputs.jobs as a YAML array.
-		if jobConfig.runsOnArray && len(jobConfig.RunsOn) > 0 {
-			// Keep []string{""} semantically unset, matching FormatRunsOn behavior.
-			if len(jobConfig.RunsOn) == 1 && jobConfig.RunsOn[0] == "" {
-				job.RunsOn = "runs-on: " + defaultRunsOn
-			} else {
-				job.RunsOn = c.indentYAMLLines(renderRunsOnSnippet([]string(jobConfig.RunsOn)), "    ")
-			}
-		} else {
-			// FormatRunsOn handles defaulting and YAML-safe rendering.
-			job.RunsOn = "runs-on: " + FormatRunsOn(jobConfig.RunsOn, defaultRunsOn)
-		}
+		// Set runs-on. Preserve list-shaped input from safe-outputs.jobs as a
+		// YAML array; formatSafeJobRunsOn centralizes the array-vs-scalar
+		// rendering decision shared with other runs-on parsers.
+		job.RunsOn = c.indentYAMLLines(formatSafeJobRunsOn(jobConfig.RunsOn, jobConfig.runsOnArray, defaultRunsOn), "    ")
 
 		// Set if condition - combine safe output type check with user-provided condition
 		// Custom safe jobs should only run if the agent output contains the job name (tool call)