[refactor] Semantic function clustering analysis: semver duplication, single-function file, and cross-package naming conflicts

[github-actions]

Analysis of 621 non-test Go source files across 18 packages in pkg/, cataloging ~8,455 functions. The codebase is generally well-organized with clear file-per-feature patterns. Three actionable refactoring opportunities were identified.

---

Executive Summary

Finding	Severity	Effort	Files Affected
Semver logic duplicated across two packages	Medium	Low	2 files
Single-function file workflow_name.go adds unnecessary indirection	Low	Very Low	2 files
Two validateWorkflowName variants with conflicting semantics	Medium	Low	2 files

---

Finding 1: Semver Logic Duplicated Across Two Packages

Issue: Semantic version handling is implemented independently in both pkg/workflow and pkg/cli, using the same underlying golang.org/x/mod/semver library with similar patterns and no code sharing.

pkg/workflow/semver.go

func isValidVersionTag(s string) bool        // Validates v1, v1.2, v1.2.3
func compareVersions(v1, v2 string) int      // Wraps semver.Compare
func isSemverCompatible(pin, requested string) bool  // Major version match check

pkg/cli/semver.go

type semanticVersion struct { major, minor, patch int; pre, raw string }
func isSemanticVersionTag(ref string) bool   // Validates via semver.IsValid
func parseVersion(v string) *semanticVersion // Full parse with canonical form
func (v *semanticVersion) isPreciseVersion() bool
func (v *semanticVersion) isNewer(other *semanticVersion) bool
```

**Overlap**: Both packages add a `v` prefix before calling `golang.org/x/mod/semver`, both initialize a package-level `semverLog = logger.New(...)`. The `isValidVersionTag` and `isSemanticVersionTag` functions address the same problem (is a string a valid semver?) with different approaches.

**Recommendation**: Extract shared semver primitives into `pkg/semver` (or `pkg/semverutil`) and have both packages delegate to it. The `parseVersion`/`semanticVersion` type from `cli` and the `isSemverCompatible` check from `workflow` could serve as the foundation.

**Estimated impact**: Eliminates divergent implementations; future semver bugs fixed in one place.

---

### Finding 2: Single-Function File `workflow/workflow_name.go`

**Issue**: `pkg/workflow/workflow_name.go` contains exactly one exported function, `SanitizeIdentifier`, which is a thin wrapper around `SanitizeName` defined in `pkg/workflow/strings.go`. The `strings.go` file already documents this function in its package-level comment:

```
// SanitizeIdentifier (workflow_name.go): Creates clean identifiers for user agents

workflow_name.go (entire non-comment body):

func SanitizeIdentifier(name string) string {
    workflowNameLog.Printf("Sanitizing workflow identifier: %s", name)
    result := SanitizeName(name, &SanitizeOptions{
        PreserveSpecialChars: []rune{},
        TrimHyphens:          true,
        DefaultValue:         "github-agentic-workflow",
    })
    if result != name {
        workflowNameLog.Printf("Sanitized identifier: %s -> %s", name, result)
    }
    return result
}

Recommendation: Move SanitizeIdentifier into pkg/workflow/strings.go where the rest of the sanitize family lives and the function is already cross-referenced. Delete workflow_name.go. The logger variable workflowNameLog should be replaced with the existing stringsLog.

Estimated impact: Removes one file, consolidates the entire sanitize function family into one file as the package docs already imply.

---

Finding 3: Two validateWorkflowName Variants with Conflicting Semantics

Issue: Two unexported functions with near-identical names exist in the same cli package with fundamentally different validation rules, creating a maintenance hazard:

pkg/cli/mcp_server_helpers.go:136 — MCP-specific, lenient

func validateWorkflowName(workflowName string) error {
    if workflowName == "" {
        return nil   // ← empty is VALID
    }
    // Tries workflow.ResolveWorkflowName first, then falls back to regex
}

pkg/cli/validators.go:18 — General, strict

func ValidateWorkflowName(s string) error {
    if s == "" {
        return errors.New("workflow name cannot be empty")  // ← empty is INVALID
    }
    if !workflowNameRegex.MatchString(s) {
        return errors.New("workflow name must contain only alphanumeric characters, hyphens, and underscores")
    }
}

The unexported validateWorkflowName in mcp_server_helpers.go does not call the exported ValidateWorkflowName from validators.go, meaning the regex validation in validators.go is silently skipped in the MCP path.

Recommendation: Rename the MCP-specific function to validateMCPWorkflowName (or similar) to make the intentional divergence explicit and searchable. Add a comment explaining why empty is valid in the MCP context. Consider whether the MCP path should also apply the character-validity regex from validators.go after the empty check.

Estimated impact: Prevents future developers from assuming both validators are equivalent; makes the semantic difference discoverable.

---

General Observations (No Action Required)

The following patterns were examined and found to be well-organized or intentional:

• WASM build-tag pairs (github_cli.go / github_cli_wasm.go, etc.): Expected pattern, not duplication.
• update_issue_helpers.go, update_pull_request_helpers.go, update_discussion_helpers.go: Each is a thin typed wrapper over parseUpdateEntityConfigTyped — correct use of generics + per-entity files.
• missing_data.go, missing_tool.go: Single-delegation files that are intentionally separate for distinct safe output types.
• compiler_safe_outputs_*.go vs safe_outputs_*.go: Distinct responsibilities (compiler YAML generation vs runtime config parsing).
• pkg/workflow/semver.go vs pkg/cli/semver.go: Currently unexported in both packages; extracting to a shared utility is recommended (Finding 1) but not blocking.
• strict_mode_*.go validation files: Well-structured domain split across env, network, permissions, sandbox.

---

Implementation Checklist

• Finding 1 (Medium): Create pkg/semver or pkg/semverutil package; migrate workflow/semver.go and cli/semver.go to use shared primitives
• Finding 2 (Low): Merge workflow/workflow_name.go:SanitizeIdentifier into workflow/strings.go; delete workflow_name.go
• Finding 3 (Medium): Rename mcp_server_helpers.go:validateWorkflowName → validateMCPWorkflowName; add comment explaining the semantic difference; evaluate whether character-validity check should be added

---

Analysis Metadata

• Go files analyzed: 621 non-test files across 18 packages in pkg/
• Functions cataloged: ~8,455
• Packages examined: pkg/cli (219 files, ~2,693 functions), pkg/workflow (304 files, ~4,789 functions), pkg/parser (37 files, ~508 functions), plus 15 utility packages
• Detection method: Serena semantic analysis + naming pattern analysis + manual cross-reference
• Analysis date: 2026-03-29

References: §23707926652

Generated by Semantic Function Refactoring · ◷

• expires on Mar 31, 2026, 11:32 AM UTC