Skip to content

[refactor] Consolidate triplicate merge helpers, sorted-map-keys reinventions, and misplaced helper clustersΒ #41337

Description

@github-actions

πŸ”§ Semantic Function Clustering Analysis

Analysis of repository: github/gh-aw β€” non-test .go files under pkg/ (956 source files, primarily pkg/workflow (414) and pkg/cli (325)).

This run focuses on three fresh, high-confidence clusters distinct from the previously-filed (now closed) truncation / file-existence / map-as-set findings: (1) byte-identical sibling functions that should collapse into one, (2) an underused local helper that is reinvented inline, and (3) functions whose purpose does not match their host file.

Key findings

# Cluster Sites Existing helper Priority
1 Triplicate merge* wrappers in role_checks.go 3 sliceutil.MergeUnique (already called) High
2 Inline slices.Sorted(maps.Keys(...)) 13 sortedMapKeys (string-only, used once) Medium
3 Outlier functions (purpose differs from file name) ~5 files n/a (move/split) Medium

1. Triplicate merge wrappers (High)

pkg/workflow/role_checks.go defines three methods that are byte-identical except for a logged label string:

func (c *Compiler) mergeSkipRoles(top []string, imported []string) []string {
    result := sliceutil.MergeUnique(top, imported...)
    if len(result) > 0 {
        roleLog.Printf("Merged %s: %v (top=%d, imported=%d, total=%d)", "skip-roles", result, len(top), len(imported), len(result))
    }
    return result
}
// mergeSkipBots  β€” identical, label "skip-bots"
// mergeBots      β€” identical, label "bots"
  • pkg/workflow/role_checks.go:627 β€” mergeSkipRoles
  • pkg/workflow/role_checks.go:636 β€” mergeSkipBots
  • pkg/workflow/role_checks.go:645 β€” mergeBots

Recommendation: collapse into a single helper that takes the label, e.g. mergeUniqueLogged(label string, top, imported []string) []string, and update the 3 call sites. Removes ~12 lines of duplicated logic and keeps the (debug) log output identical. Lowest-risk consolidation in this report.


2. Inline slices.Sorted(maps.Keys(...)) reinventions (Medium)

A local helper already exists β€” pkg/workflow/map_helpers.go:72 sortedMapKeys(m map[string]string) []string β€” but it is only used once, while the same one-liner is inlined 13 times across 5 files. Because the inline sites use varying map value types, sortedMapKeys (string-valued only) cannot absorb them as-is.

Call sites (13)
  • pkg/workflow/domains.go β€” 8 occurrences (lines 362, 428, 523, 732, 859, 925, 1012, 1053)
  • pkg/workflow/compiler_yaml_step_conversion.go:156, :197
  • pkg/workflow/compiler_main_job.go:112
  • pkg/workflow/map_helpers.go:73 (inside sortedMapKeys itself)
  • pkg/cli/run_workflow_validation.go

Recommendation: generalize the helper to func SortedKeys[K cmp.Ordered, V any](m map[K]V) []K { return slices.Sorted(maps.Keys(m)) } (a natural fit for pkg/sliceutil or a new maputil), then replace the 13 inline sites. The standalone sortedMapKeys can then delegate or be removed. Centralizes deterministic-ordering intent and makes it discoverable.


3. Outlier functions β€” purpose does not match host file (Medium)

Functions clustered into files whose name implies a different responsibility. Each is a candidate move/split to restore the one-file-per-feature rule. Per-site judgment recommended (these are organizational, not behavioral, changes).

Strongest examples
  • pkg/cli/token_usage.go β€” 5 file-discovery functions (findTokenUsageFile, findAgentUsageFile, findUsageJSONLFiles, findAPIProxyEventsFile, findAgentStdioFile) living in a token-usage parsing/summarization file. Suggest extracting run-directory file discovery into its own file (e.g. run_dir_files.go).
  • pkg/workflow/template_injection_utils.go β€” shell-script parsing helpers (extractRunBlocks, stripShellLineComments, removeHeredocContent) in a file named for template-injection validation. Suggest a shell_script_utils.go.
  • pkg/cli/compile_schedule_calendar.go β€” cron parsing (parseCronField, parseCronFieldPart, parseCronSchedule) inside a calendar/heatmap rendering file. Suggest separating parse from render (cron_parser.go).
  • pkg/workflow/expression_patterns.go β€” boolean predicates (hasExpressionMarker, containsExpression, isExpression) in a file documented as the single source of truth for regex pattern definitions. Suggest an expression_utils.go.
  • pkg/cli/vscode_config.go β€” ensureVSCodeSettings performs filesystem I/O / directory creation alongside JSON (un)marshaling. Suggest a vscode_setup.go.

Recommendation: treat as a low-risk organizational sweep; move outliers to purpose-named files and keep behavior unchanged. Prioritize token_usage.go (5 functions β€” clearest win).


Notes & non-findings

  • The mutable* family in pkg/cli/outcome_eval_update.go (mutableString, mutableBool, mutableStringSlice) initially looked like a generics candidate, but each body has genuinely distinct conversion logic β€” not a mechanical generics collapse. Left out intentionally.
  • The util packages (sliceutil, stringutil, fileutil, typeutil, setutil) are already well-used; this report targets the gaps where helpers exist but are bypassed, plus organizational drift.

Analysis metadata

  • Go files analyzed: 956 non-test source files under pkg/
  • Method: Serena/grep symbol inventory + naming-pattern clustering + body comparison across pkg/workflow, pkg/cli
  • Clusters reported: 3 (1 High, 2 Medium)
  • Confirmed duplicate functions: 3 (mergeSkipRoles/mergeSkipBots/mergeBots)
  • Reinvention sites: 13 (slices.Sorted(maps.Keys(...)))
  • Analysis date: 2026-06-25

Generated by πŸ”§ Semantic Function Refactoring Β· 191.2 AIC Β· βŒ– 15 AIC Β· ⊞ 9.2K Β· β—·

  • expires on Jun 26, 2026, 4:12 PM UTC-08:00

Metadata

Metadata

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions