[refactor] Semantic Function Clustering Analysis: Outlier and Misplaced Functions

[github-actions]

Overview

Semantic analysis of 614 non-test Go source files across pkg/ (2,886 functions total) identified several concrete refactoring opportunities. The codebase is well-organized overall — the strategy-pattern engine hierarchy (claude/codex/gemini/copilot), the update/close entity helpers, and the MCP config rendering layers are all intentional and clean. The findings below focus on actionable outliers that reduce discoverability and add accidental complexity.

---

Critical Issues

1. Test-Only Functions Compiled Into Production Binary

File: pkg/workflow/compiler_test_helpers.go
Issue: This file is named like a test helper but does not end in _test.go. All three functions it defines are called exclusively from *_test.go files (59 call sites), yet they are compiled into the production binary.

// compiler_test_helpers.go — NOT a test file, but only used by tests
func containsInNonCommentLines(content, search string) bool { ... }
func indexInNonCommentLines(content, search string) int { ... }
func extractJobSection(yamlContent, jobName string) string { ... }
```

**Recommendation:** Rename to `compiler_test_helpers_test.go` (or move functions into an existing `_test.go` file in the package). This removes dead production code and makes the intent explicit.

**Estimated Effort:** 15–30 minutes  
**Impact:** Removes 3 unexported functions from production binary; clarifies test vs. production boundary.

---

#### 2. Sanitize Functions Scattered Across Compiler Files

The package has a dedicated home for string sanitization — `strings.go` and `workflow_name.go` — with `SanitizeName` as the common base. However, additional sanitize functions exist as outliers buried in domain-specific compiler files:

| Function | Current File | Issue |
|----------|-------------|-------|
| `SanitizeWorkflowIDForCacheKey` | `compiler_yaml_helpers.go` | String processing in a YAML compilation file |
| `sanitizeJobName` | `compiler_safe_output_jobs.go` | String processing in safe-output generation file |
| `sanitizeRefForPath` | `compiler_yaml_main_job.go` | String processing in main-job compiler file |

<details>
<summary><b>Current vs. Recommended Locations</b></summary>

```
pkg/workflow/strings.go               ← primary home for sanitize functions
  SanitizeName (configurable base)
  SanitizeWorkflowName

pkg/workflow/workflow_name.go         ← workflow-identity sanitizers
  SanitizeIdentifier

pkg/workflow/compiler_yaml_helpers.go ← YAML compilation helpers (outlier here)
  SanitizeWorkflowIDForCacheKey       ← should move to strings.go or workflow_name.go

pkg/workflow/compiler_safe_output_jobs.go ← (outlier here)
  sanitizeJobName                     ← should move to strings.go

pkg/workflow/compiler_yaml_main_job.go ← (outlier here)
  sanitizeRefForPath                  ← should move to strings.go

Notably, strings.go even includes a doc comment listing the canonical sanitize functions, but does not list SanitizeWorkflowIDForCacheKey — a sign it was added in the wrong file.

Recommendation: Move SanitizeWorkflowIDForCacheKey, sanitizeJobName, and sanitizeRefForPath to strings.go (or workflow_name.go for the identifier-focused ones). Update call sites accordingly (no signature changes needed).

Estimated Effort: 1–2 hours
Impact: Centralizes string processing; new developers can find all sanitization logic in one place.

---

3. Redundant Method Wrapper in compiler_yaml_helpers.go

File: pkg/workflow/compiler_yaml_helpers.go
Issue: A public free function and a private method that wraps it identically exist side by side:

// Public free function (used by external callers)
func ConvertStepToYAML(stepMap map[string]any) (string, error) {
    // ... ~30 lines of logic
}

// Private method — adds zero value, just delegates
func (c *Compiler) convertStepToYAML(stepMap map[string]any) (string, error) {
    return ConvertStepToYAML(stepMap)
}

Recommendation: Remove (*Compiler).convertStepToYAML and replace its call sites with direct calls to ConvertStepToYAML. The method adds no context from the Compiler receiver.

Estimated Effort: 30–60 minutes
Impact: Reduces redundancy; removes a confusing method that implies it does something the free function doesn't.

---

4. Dual Version Management in pkg/workflow

Files: pkg/workflow/version.go and pkg/workflow/compiler_types.go
Issue: Two separate global version variables with similar-sounding setter functions:

// version.go
var compilerVersion = "dev"         // used in generated workflow headers
func SetVersion(v string)           // sets compilerVersion
func GetVersion() string            // returns compilerVersion

// compiler_types.go
var defaultVersion = "dev"          // used when creating new Compiler instances
func SetDefaultVersion(version string) // sets defaultVersion
```

These are distinct concerns but `SetVersion` is never called externally (only `SetDefaultVersion` is called by the CLI), making the relationship between the two variables non-obvious. Additionally, `(*Compiler).GetVersion()` in `compiler_types.go` returns `c.version` (the instance field), while the package-level `GetVersion()` in `version.go` returns `compilerVersion` — same name, different values.

<details>
<summary><b>Call graph showing the confusion</b></summary>

```
CLI init:
  cli.SetVersionInfo(v)
    → workflow.SetDefaultVersion(v)  [sets defaultVersion]
    (SetVersion is NOT called here)

Compiler creation (NewCompiler):
  c.version = defaultVersion         [copies defaultVersion into instance field]

workflow.GetVersion():
  return compilerVersion             [returns compilerVersion, not defaultVersion!]
  → used in: header.go, gemini_engine.go, claude_engine.go, etc.

(*Compiler).GetVersion():
  return c.version                   [returns instance field (= defaultVersion)]

In practice, compilerVersion stays "dev" while defaultVersion gets the real version — but header.go uses GetVersion() (the package-level one), which means generated workflow headers never get the real version unless SetVersion is also called.

Recommendation: Audit whether compilerVersion / SetVersion / package-level GetVersion() are still needed, or whether everything should route through defaultVersion. Consolidate into a single version variable in version.go and document the initialization flow clearly.

Estimated Effort: 2–3 hours (includes verification that headers work correctly)
Impact: Eliminates potential subtle bugs where generated YAML headers always show "dev"; removes confusing dual-variable pattern.

---

Medium-Impact Findings

5. `compiler_yaml_helpers.go` Is a Catch-All File

Despite its name ("yaml helpers"), compiler_yaml_helpers.go contains functions spanning multiple distinct concerns:

Function	Concern
ContainsCheckout	Step presence detection
GetWorkflowIDFromPath	Workflow metadata extraction
SanitizeWorkflowIDForCacheKey	String processing (outlier)
ConvertStepToYAML	YAML marshalling
convertStepToYAML	Redundant wrapper (see #3 above)
unquoteUsesWithComments	YAML post-processing
getInstallationVersion	Engine version resolution
getDefaultAgentModel	Engine model defaults
versionToGitRef	Version string conversion
generateCheckoutActionsFolder	Code generation
generateSetupStep	Code generation
generateSetRuntimePathsStep	Code generation
renderStepFromMap	YAML rendering

Recommendation: The file could be split into:

• compiler_yaml_marshalling.go — YAML conversion/rendering functions
• compiler_step_generation.go — step generation functions
• String processing functions → strings.go

This is lower priority since the current organization works; it mainly affects code navigation.

6. `filterMapKeys` in `map_helpers.go` vs `FilterMapKeys` in `sliceutil`

Two functions with nearly identical names but different signatures and purposes:

// pkg/workflow/map_helpers.go — returns filtered map (excludes specific keys)
func filterMapKeys(original map[string]any, excludeKeys ...string) map[string]any

// pkg/sliceutil/sliceutil.go — returns slice of keys matching predicate
func FilterMapKeys[K comparable, V any](m map[K]V, predicate func(K, V) bool) []K

These serve genuinely different use cases and are not true duplicates. However, the naming collision can cause confusion for developers looking for a "filter map keys" utility. Consider renaming the workflow-internal function to excludeMapKeys to clarify intent.

---

Implementation Checklist

• [High] Rename compiler_test_helpers.go → compiler_test_helpers_test.go
• [High] Move SanitizeWorkflowIDForCacheKey from compiler_yaml_helpers.go to strings.go (or workflow_name.go)
• [High] Move sanitizeJobName from compiler_safe_output_jobs.go to strings.go
• [High] Move sanitizeRefForPath from compiler_yaml_main_job.go to strings.go
• [High] Remove (*Compiler).convertStepToYAML wrapper method, replace call sites with ConvertStepToYAML
• [Medium] Audit and consolidate the dual version management in version.go vs compiler_types.go
• [Low] Consider renaming filterMapKeys → excludeMapKeys to avoid naming collision with sliceutil.FilterMapKeys
• [Low] Consider splitting compiler_yaml_helpers.go by concern

---

Analysis Metadata

Metric	Value
Total Go files analyzed	614
Total functions catalogued	2,886
Packages analyzed	18
Outlier functions identified	5
Duplicate/redundant functions	1
Version management confusion	1 pattern
Detection method	Serena LSP + naming pattern analysis
Analysis date	2026-03-25

References:

• §23538710186

Generated by Semantic Function Refactoring · ◷

• expires on Mar 27, 2026, 11:39 AM UTC

[pelikhan]

/plan

[github-actions]

🍪 Issue Monster has assigned this to Copilot!

I've identified this issue as a good candidate for automated resolution and assigned it to the Copilot coding agent.

The Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · ◷