refactor: SortedKeys sweep, deduplicate dedup logic, redistribute misplaced helpers (#41829)

Author: Copilot

docs/adr/41829-consolidate-sliceutil-sorted-keys-and-redistribute-helpers.md

@@ -0,0 +1,49 @@
+# ADR-41829: Consolidate `sliceutil.SortedKeys` as Canonical Map-Key Utility and Redistribute Misplaced Helpers
+
+**Date**: 2026-06-27
+**Status**: Draft
+**Deciders**: Unknown (automated refactor by copilot-swe-agent)
+
+---
+
+### Context
+
+The codebase had accumulated a widespread manual pattern for collecting and sorting map keys: `make → append → sort.Strings`. This idiom was duplicated at 40+ sites across `pkg/cli`, `pkg/workflow`, and `pkg/parser`, even though `sliceutil.SortedKeys` already existed and served this exact purpose. Separately, several general-purpose helper functions had drifted into files where they did not belong semantically — for example, `isDescendant` (a generic YAML-indent helper) lived in `codemod_github_app.go`, and safe-output helpers accumulated in `notify_comment.go`. This created maintenance friction: new contributors reinvented the utility, and unrelated helpers were hard to discover in their misplaced locations.
+
+### Decision
+
+We will adopt `sliceutil.SortedKeys` as the single canonical way to collect and sort map keys throughout the codebase, replacing all manual `make → append → sort.Strings` occurrences. Concurrently, we will enforce module cohesion by relocating misplaced helper functions to files whose names reflect their domain — generic YAML helpers to `yaml_frontmatter_utils.go`, engine-to-API-host logic to `engine_api_targets.go`, and safe-output env-var construction to `safe_outputs_env.go`. The duplicate `toEnvVarCase` function is removed in favour of the existing `normalizeJobNameForEnvVar`, which covers the same cases.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep the manual `make → append → sort.Strings` idiom
+
+Every callsite continues to inline the three-step collect-then-sort pattern. This requires no refactor effort and avoids touching 59 files. Rejected because the duplication had already reached 40+ sites and was growing; new callsites were consistently reinventing the pattern rather than discovering the existing utility, confirming the drift would continue unchecked.
+
+#### Alternative 2: Use stdlib `maps.Keys` + `slices.Sort` (Go 1.21+)
+
+Replace the manual idiom with `maps.Keys(m)` followed by `slices.Sort(...)`. This is idiomatic stdlib and requires no project-specific utility. Rejected because `sliceutil.SortedKeys` is already established in the project and composes both steps in one call, reducing boilerplate to a single expression. Migrating to a new stdlib pair would only exchange one two-line idiom for another without adding the readability gain of the single-call form; it would also invalidate any existing usages of `sliceutil.SortedKeys` elsewhere.
+
+#### Alternative 3: Address only the deduplication fix in `cache_integrity.go`; leave the sweep for a later PR
+
+A narrow fix to the inconsistency in `canonicalReposScope` (which hand-rolled dedup while `sliceutil.Deduplicate` was called on the same file) could be merged independently. Rejected because the broader sweep was machine-generated and low-risk; deferring it would leave the 40+ duplicated sites in place with no clear owner to address them later.
+
+### Consequences
+
+#### Positive
+- 725 lines deleted, 302 added — net −423 lines of boilerplate across 66 files, improving signal-to-noise ratio in the codebase.
+- A single canonical pattern for sorted map iteration eliminates the "which way should I do this?" question for contributors.
+- Misplaced helpers are now co-located with semantically related code, making them discoverable and reducing surprise when reading individual files.
+- Removing the `toEnvVarCase` duplicate eliminates a potential divergence point if the normalization logic ever needs to change.
+
+#### Negative
+- `sliceutil` is now a transitive import dependency for many more packages; any future breaking change to `SortedKeys` (signature, behaviour, or package path) requires updating all 59+ files.
+- The sweep is mechanical and broad; reviewers must verify that no callsite had intentional non-lexicographic ordering or special nil-vs-empty semantics that `SortedKeys` does not preserve (e.g., `update_container_pins.go` adds an explicit `nil → []string{}` guard after the replacement).
+
+#### Neutral
+- The `import` block in many files changes from `"sort"` to `"github.com/github/gh-aw/pkg/sliceutil"`; tooling (goimports, linters) must be aware of the project-internal package path.
+- Function relocation changes which file a `git blame` points to for `isDescendant`, `isExpressionValue`, `getEngineAPIHosts`, `buildSafeOutputJobsEnvVars`, and `systemSafeOutputJobNames` — history is preserved via `git log --follow`.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/audit_comparison.go

@@ -8,11 +8,11 @@ import (
 	"os"
 	"path/filepath"
 	"slices"
-	"sort"
 	"strings"
 
 	"github.com/github/gh-aw/pkg/constants"
 	"github.com/github/gh-aw/pkg/logger"
+	"github.com/github/gh-aw/pkg/sliceutil"
 	"github.com/github/gh-aw/pkg/workflow"
 )
 
@@ -466,11 +466,7 @@ func collectMCPFailureServers(failures []MCPFailureReport) []string {
 		serverSet[failure.ServerName] = struct{}{}
 	}
 
-	servers := make([]string, 0, len(serverSet))
-	for server := range serverSet {
-		servers = append(servers, server)
-	}
-	sort.Strings(servers)
+	servers := sliceutil.SortedKeys(serverSet)
 	return servers
 }
 

pkg/cli/audit_cross_run.go

@@ -1,10 +1,10 @@
 package cli
 
 import (
-	"sort"
 	"time"
 
 	"github.com/github/gh-aw/pkg/logger"
+	"github.com/github/gh-aw/pkg/sliceutil"
 	"github.com/github/gh-aw/pkg/stats"
 )
 
@@ -327,11 +327,7 @@ func buildCrossRunMCPHealth(mcpServerMap map[string]*crossRunMCPServerAgg, total
 
 	auditCrossRunLog.Printf("Building MCP health for %d server(s) across %d run(s)", len(mcpServerMap), totalRuns)
 
-	sortedServers := make([]string, 0, len(mcpServerMap))
-	for name := range mcpServerMap {
-		sortedServers = append(sortedServers, name)
-	}
-	sort.Strings(sortedServers)
+	sortedServers := sliceutil.SortedKeys(mcpServerMap)
 
 	health := make([]MCPServerCrossRunHealth, 0, len(sortedServers))
 	for _, name := range sortedServers {
@@ -370,11 +366,7 @@ func buildCrossRunDomainInventory(
 	domainMap map[string]*crossRunDomainAgg,
 	runIDs []int64,
 ) {
-	sortedDomains := make([]string, 0, len(domainMap))
-	for domain := range domainMap {
-		sortedDomains = append(sortedDomains, domain)
-	}
-	sort.Strings(sortedDomains)
+	sortedDomains := sliceutil.SortedKeys(domainMap)
 
 	auditCrossRunLog.Printf("Building domain inventory: %d unique domain(s) across %d run(s)", len(sortedDomains), len(runIDs))
 

pkg/cli/audit_diff.go

@@ -6,12 +6,12 @@ import (
 	"fmt"
 	"math"
 	"path/filepath"
-	"sort"
 	"strings"
 	"time"
 
 	"github.com/github/gh-aw/pkg/constants"
 	"github.com/github/gh-aw/pkg/logger"
+	"github.com/github/gh-aw/pkg/sliceutil"
 )
 
 var auditDiffLog = logger.New("cli:audit_diff")
@@ -92,11 +92,7 @@ func computeFirewallDiff(run1ID, run2ID int64, run1, run2 *FirewallAnalysis) *Fi
 	}
 
 	// Sorted domain list for deterministic output
-	sortedDomains := make([]string, 0, len(allDomains))
-	for domain := range allDomains {
-		sortedDomains = append(sortedDomains, domain)
-	}
-	sort.Strings(sortedDomains)
+	sortedDomains := sliceutil.SortedKeys(allDomains)
 
 	anomalyCount := 0
 
@@ -444,11 +440,7 @@ func computeMCPToolsDiff(run1, run2 *MCPToolUsageData) *MCPToolsDiff {
 		allKeys[k] = struct{}{}
 	}
 
-	sortedKeys := make([]string, 0, len(allKeys))
-	for k := range allKeys {
-		sortedKeys = append(sortedKeys, k)
-	}
-	sort.Strings(sortedKeys)
+	sortedKeys := sliceutil.SortedKeys(allKeys)
 
 	diff := &MCPToolsDiff{}
 	anomalyCount := 0
@@ -649,11 +641,7 @@ func computeToolCallsDiff(m1, m2 *LogMetrics) *ToolCallsDiff {
 		allNames[k] = struct{}{}
 	}
 
-	sortedNames := make([]string, 0, len(allNames))
-	for k := range allNames {
-		sortedNames = append(sortedNames, k)
-	}
-	sort.Strings(sortedNames)
+	sortedNames := sliceutil.SortedKeys(allNames)
 
 	diff := &ToolCallsDiff{}
 	var run1Total, run2Total int
@@ -756,11 +744,7 @@ func computeBashCommandsDiff(run1Tools, run2Tools map[string]ToolCallInfo) *Bash
 		return nil
 	}
 
-	sortedNames := make([]string, 0, len(allNames))
-	for k := range allNames {
-		sortedNames = append(sortedNames, k)
-	}
-	sort.Strings(sortedNames)
+	sortedNames := sliceutil.SortedKeys(allNames)
 
 	bashDiff := &BashCommandsDiff{}
 	for _, name := range sortedNames {

pkg/cli/audit_expanded.go

@@ -7,12 +7,12 @@ import (
 	"os"
 	"path/filepath"
 	"slices"
-	"sort"
 	"strings"
 	"time"
 
 	"github.com/github/gh-aw/pkg/fileutil"
 	"github.com/github/gh-aw/pkg/logger"
+	"github.com/github/gh-aw/pkg/sliceutil"
 	"github.com/github/gh-aw/pkg/timeutil"
 	"github.com/github/gh-aw/pkg/workflow"
 )
@@ -662,10 +662,6 @@ func extractMCPServerNamesFromAwInfo(logsPath string) ([]string, bool) {
 		return nil, false
 	}
 
-	names := make([]string, 0, len(servers))
-	for name := range servers {
-		names = append(names, name)
-	}
-	sort.Strings(names)
+	names := sliceutil.SortedKeys(servers)
 	return names, len(names) > 0
 }

pkg/cli/audit_report_experiments.go

@@ -10,11 +10,11 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"sort"
 	"strings"
 
 	"github.com/github/gh-aw/pkg/constants"
 	"github.com/github/gh-aw/pkg/logger"
+	"github.com/github/gh-aw/pkg/sliceutil"
 )
 
 var experimentDataLog = logger.New("cli:audit_report_experiments")
@@ -112,11 +112,7 @@ func extractExperimentData(logsPath string) *ExperimentData {
 	// Derive this-run assignments: the variant selected on the most-recent run is
 	// the one with the maximum count (ties resolved by sorted order).
 	assignments := make(map[string]string, len(state.Counts))
-	names := make([]string, 0, len(state.Counts))
-	for name := range state.Counts {
-		names = append(names, name)
-	}
-	sort.Strings(names)
+	names := sliceutil.SortedKeys(state.Counts)
 
 	for _, name := range names {
 		variantCounts := state.Counts[name]
@@ -145,11 +141,7 @@ func formatExperimentLabel(exp *ExperimentData) string {
 		return ""
 	}
 
-	names := make([]string, 0, len(exp.Assignments))
-	for name := range exp.Assignments {
-		names = append(names, name)
-	}
-	sort.Strings(names)
+	names := sliceutil.SortedKeys(exp.Assignments)
 
 	parts := make([]string, 0, len(names))
 	for _, name := range names {
@@ -196,11 +188,7 @@ func deriveLastSelectedVariant(variantCounts map[string]int) string {
 		return ""
 	}
 
-	variants := make([]string, 0, len(variantCounts))
-	for v := range variantCounts {
-		variants = append(variants, v)
-	}
-	sort.Strings(variants)
+	variants := sliceutil.SortedKeys(variantCounts)
 
 	selected := variants[0]
 	maxCount := variantCounts[selected]

pkg/cli/codemod_dependabot_permissions.go

@@ -6,6 +6,7 @@ import (
 	"strings"
 
 	"github.com/github/gh-aw/pkg/logger"
+	"github.com/github/gh-aw/pkg/sliceutil"
 	"github.com/github/gh-aw/pkg/workflow"
 )
 
@@ -241,10 +242,6 @@ func sortedMissingPermissionKeys(missing map[workflow.PermissionScope]workflow.P
 }
 
 func sortedRemainingPermissionKeys(remaining map[string]workflow.PermissionLevel) []string {
-	keys := make([]string, 0, len(remaining))
-	for key := range remaining {
-		keys = append(keys, key)
-	}
-	sort.Strings(keys)
+	keys := sliceutil.SortedKeys(remaining)
 	return keys
 }

pkg/cli/codemod_effective_tokens_to_ai_credits.go

@@ -137,10 +137,6 @@ func convertEffectiveTokensToAICredits(effectiveTokens int) (string, bool) {
 	return strconv.Itoa(aiCredits), true
 }
 
-func isExpressionValue(value string) bool {
-	return strings.Contains(value, "${{") && strings.Contains(value, "}}")
-}
-
 func rewriteTopLevelScalarLine(line, newKey, normalizedValue string) string {
 	indent := getIndentation(line)
 	parts := strings.SplitN(line, ":", 2)

pkg/cli/codemod_github_app.go

@@ -179,10 +179,3 @@ func renameAppToGitHubApp(lines []string) ([]string, bool) {
 
 	return result, modified
 }
-
-// isDescendant returns true if childIndent is deeper (more indented) than parentIndent.
-// It is used as a "belongs to this block" check — any line more indented than the parent
-// is treated as being within the parent's scope.
-func isDescendant(childIndent, parentIndent string) bool {
-	return len(childIndent) > len(parentIndent)
-}

pkg/cli/experiments_analyze_statistics.go

@@ -4,11 +4,11 @@ import (
 	"fmt"
 	"math"
 	"os"
-	"sort"
 	"strings"
 
 	"github.com/github/gh-aw/pkg/console"
 	"github.com/github/gh-aw/pkg/logger"
+	"github.com/github/gh-aw/pkg/sliceutil"
 	"github.com/github/gh-aw/pkg/workflow"
 )
 
@@ -130,11 +130,7 @@ func computeExperimentAnalysis(exp ExperimentVariantStats, cfg *workflow.Experim
 	}
 
 	// Collect variant names in alphabetical order for deterministic output.
-	variantNames := make([]string, 0, len(exp.Variants))
-	for name := range exp.Variants {
-		variantNames = append(variantNames, name)
-	}
-	sort.Strings(variantNames)
+	variantNames := sliceutil.SortedKeys(exp.Variants)
 
 	// Compute expected proportions (weighted or equal split).
 	expectedPcts := expectedProportions(variantNames, cfg)