refactor: consolidate semver utilities, merge single-function file, disambiguate MCP validator

pkg/cli/mcp_server_helpers.go

@@ -126,15 +126,17 @@ func hasWriteAccess(permission string) bool {
 	}
 }
 
-// validateWorkflowName validates that a workflow name exists in the repository.
+// validateMCPWorkflowName validates that a workflow name exists in the repository.
 // Returns nil if the workflow exists, or an error with suggestions if not.
-// Empty workflow names are considered valid (means all workflows).
+// Empty workflow names are considered valid (means "all workflows").
 //
-// Note: This function checks whether a workflow exists/is accessible, not its format.
-// For format-only validation (alphanumeric characters, hyphens, underscores),
-// use validators.go:ValidateWorkflowName instead.
-func validateWorkflowName(workflowName string) error {
-	// Empty workflow name means "all workflows" - this is valid
+// Note: Unlike ValidateWorkflowName in validators.go (which enforces strict format
+// rules and rejects empty names), this MCP-specific function accepts empty names
+// because in the MCP context an empty workflow name is a valid wildcard meaning
+// "apply to all workflows". It also performs existence checks rather than format
+// checks, delegating to workflow.ResolveWorkflowName and the live workflow list.
+func validateMCPWorkflowName(workflowName string) error {
+	// Empty workflow name means "all workflows" - this is valid in the MCP context
 	if workflowName == "" {
 		return nil
 	}

pkg/cli/mcp_server_helpers_test.go

@@ -109,8 +109,8 @@ func TestHasWriteAccess(t *testing.T) {
 
 func TestValidateWorkflowName_Empty(t *testing.T) {
 	// Empty workflow name is always valid (means "all workflows")
-	if err := validateWorkflowName(""); err != nil {
-		t.Errorf("validateWorkflowName(\"\") returned error: %v", err)
+	if err := validateMCPWorkflowName(""); err != nil {
+		t.Errorf("validateMCPWorkflowName(\"\") returned error: %v", err)
 	}
 }
 

pkg/cli/mcp_server_workflow_validation_test.go

@@ -42,7 +42,7 @@ func TestMCPValidateWorkflowName(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := validateWorkflowName(tt.workflowName)
+			err := validateMCPWorkflowName(tt.workflowName)
 
 			if tt.shouldSucceed {
 				assert.NoError(t, err, "Validation should succeed for workflow: %s", tt.workflowName)

pkg/cli/mcp_tools_privileged.go

@@ -89,7 +89,7 @@ return a schema description instead of the full output. Adjust the 'max_tokens'
 		}
 
 		// Validate workflow name before executing command
-		if err := validateWorkflowName(args.WorkflowName); err != nil {
+		if err := validateMCPWorkflowName(args.WorkflowName); err != nil {
 			mcpLog.Printf("Workflow name validation failed: %v", err)
 			return nil, nil, newMCPError(jsonrpc.CodeInvalidParams, err.Error(), map[string]any{
 				"workflow_name": args.WorkflowName,

pkg/cli/remote_workflow_test.go

@@ -410,7 +410,6 @@ imports:
 	assert.Empty(t, entries, "uses: form workflowspec imports should not be downloaded")
 }
 
-
 func TestFetchAndSaveRemoteFrontmatterImports_NoImportsNoOpTracker(t *testing.T) {
 	// Build a minimal FileTracker without calling NewFileTracker (which requires a real
 	// git repository). We only need the tracking lists populated.

pkg/cli/semver.go

@@ -1,12 +1,10 @@
 package cli
 
 import (
-	"strconv"
 	"strings"
 
-	"golang.org/x/mod/semver"
-
 	"github.com/github/gh-aw/pkg/logger"
+	"github.com/github/gh-aw/pkg/semverutil"
 )
 
 var semverLog = logger.New("cli:semver")
@@ -23,59 +21,25 @@ type semanticVersion struct {
 // isSemanticVersionTag checks if a ref string looks like a semantic version tag
 // Uses golang.org/x/mod/semver for proper semantic version validation
 func isSemanticVersionTag(ref string) bool {
-	// Ensure ref has 'v' prefix for semver package
-	if !strings.HasPrefix(ref, "v") {
-		ref = "v" + ref
-	}
-	return semver.IsValid(ref)
+	return semverutil.IsValid(ref)
 }
 
 // parseVersion parses a semantic version string
 // Uses golang.org/x/mod/semver for proper semantic version parsing
 func parseVersion(v string) *semanticVersion {
 	semverLog.Printf("Parsing semantic version: %s", v)
-	// Ensure version has 'v' prefix for semver package
-	if !strings.HasPrefix(v, "v") {
-		v = "v" + v
-	}
-
-	// Check if valid semantic version
-	if !semver.IsValid(v) {
+	parsed := semverutil.ParseVersion(v)
+	if parsed == nil {
 		semverLog.Printf("Invalid semantic version: %s", v)
 		return nil
 	}
-
-	ver := &semanticVersion{raw: strings.TrimPrefix(v, "v")}
-
-	// Use semver.Canonical to get normalized version
-	canonical := semver.Canonical(v)
-
-	// Parse major, minor, patch from canonical form
-	// Strip prerelease and build metadata before splitting, since semver.Canonical
-	// preserves the prerelease suffix (e.g. "v1.2.3-beta.1" stays "v1.2.3-beta.1")
-	corePart := strings.TrimPrefix(canonical, "v")
-	if idx := strings.IndexAny(corePart, "-+"); idx >= 0 {
-		corePart = corePart[:idx]
-	}
-	parts := strings.Split(corePart, ".")
-	// Parse the numeric components; strconv.Atoi returns 0 on error, matching
-	// the previous behavior where non-numeric input produced 0.
-	if len(parts) >= 1 {
-		ver.major, _ = strconv.Atoi(parts[0])
-	}
-	if len(parts) >= 2 {
-		ver.minor, _ = strconv.Atoi(parts[1])
+	return &semanticVersion{
+		major: parsed.Major,
+		minor: parsed.Minor,
+		patch: parsed.Patch,
+		pre:   parsed.Pre,
+		raw:   parsed.Raw,
 	}
-	if len(parts) >= 3 {
-		ver.patch, _ = strconv.Atoi(parts[2])
-	}
-
-	// Get prerelease if any
-	prerelease := semver.Prerelease(v)
-	// semver.Prerelease includes the leading hyphen, strip it
-	ver.pre = strings.TrimPrefix(prerelease, "-")
-
-	return ver
 }
 
 // isPreciseVersion returns true if this version has explicit minor and patch components
@@ -94,14 +58,7 @@ func (v *semanticVersion) isPreciseVersion() bool {
 // isNewer returns true if this version is newer than the other
 // Uses golang.org/x/mod/semver.Compare for proper semantic version comparison
 func (v *semanticVersion) isNewer(other *semanticVersion) bool {
-	// Ensure versions have 'v' prefix for semver package
-	v1 := "v" + v.raw
-	v2 := "v" + other.raw
-
-	// Use semver.Compare for comparison
-	result := semver.Compare(v1, v2)
-
-	isNewer := result > 0
+	isNewer := semverutil.Compare(v.raw, other.raw) > 0
 	semverLog.Printf("Version comparison: %s vs %s, isNewer=%v", v.raw, other.raw, isNewer)
 	return isNewer
 }

pkg/semverutil/semverutil.go

@@ -0,0 +1,157 @@
+// Package semverutil provides shared semantic versioning primitives used across
+// the pkg/workflow and pkg/cli packages. Centralizing these helpers ensures that
+// semver parsing, comparison, and compatibility logic is fixed in one place.
+//
+// Both workflow and cli packages previously duplicated the "ensure v-prefix" pattern
+// and independently called golang.org/x/mod/semver. This package provides the
+// canonical implementations so both packages can delegate here.
+package semverutil
+
+import (
+	"regexp"
+	"strconv"
+	"strings"
+
+	"github.com/github/gh-aw/pkg/logger"
+	"golang.org/x/mod/semver"
+)
+
+var log = logger.New("semverutil:semverutil")
+
+// actionVersionTagRegex matches version tags: vmajor, vmajor.minor, or vmajor.minor.patch.
+// It intentionally excludes prerelease and build-metadata suffixes because GitHub Actions
+// version pins use only these three forms.
+var actionVersionTagRegex = regexp.MustCompile(`^v[0-9]+(\.[0-9]+(\.[0-9]+)?)?$`)
+
+// SemanticVersion represents a parsed semantic version.
+type SemanticVersion struct {
+	Major int
+	Minor int
+	Patch int
+	Pre   string
+	Raw   string
+}
+
+// EnsureVPrefix returns v with a leading "v" added if it is not already present.
+// The golang.org/x/mod/semver package requires the "v" prefix; callers that may
+// receive bare version strings (e.g. "1.2.3") should normalise via this helper.
+func EnsureVPrefix(v string) string {
+	if !strings.HasPrefix(v, "v") {
+		return "v" + v
+	}
+	return v
+}
+
+// IsActionVersionTag reports whether s is a valid GitHub Actions version tag.
+// Accepted formats are vmajor, vmajor.minor, and vmajor.minor.patch; prerelease
+// and build-metadata suffixes are not accepted.
+func IsActionVersionTag(s string) bool {
+	return actionVersionTagRegex.MatchString(s)
+}
+
+// IsValid reports whether ref is a valid semantic version string.
+// It uses golang.org/x/mod/semver and accepts any valid semver, including
+// prerelease and build-metadata suffixes. A bare version without a leading "v"
+// is also accepted (the prefix is added internally).
+func IsValid(ref string) bool {
+	return semver.IsValid(EnsureVPrefix(ref))
+}
+
+// ParseVersion parses v into a SemanticVersion.
+// It returns nil if v is not a valid semantic version string.
+func ParseVersion(v string) *SemanticVersion {
+	log.Printf("Parsing semantic version: %s", v)
+	v = EnsureVPrefix(v)
+
+	if !semver.IsValid(v) {
+		log.Printf("Invalid semantic version: %s", v)
+		return nil
+	}
+
+	ver := &SemanticVersion{Raw: strings.TrimPrefix(v, "v")}
+
+	// Use semver.Canonical to get normalized version
+	canonical := semver.Canonical(v)
+
+	// Strip prerelease and build metadata before splitting, since semver.Canonical
+	// preserves the prerelease suffix (e.g. "v1.2.3-beta.1" stays "v1.2.3-beta.1")
+	corePart := strings.TrimPrefix(canonical, "v")
+	if idx := strings.IndexAny(corePart, "-+"); idx >= 0 {
+		corePart = corePart[:idx]
+	}
+	parts := strings.Split(corePart, ".")
+	// Parse the numeric components; strconv.Atoi returns 0 on error, matching
+	// the previous behavior where non-numeric input produced 0.
+	if len(parts) >= 1 {
+		ver.Major, _ = strconv.Atoi(parts[0])
+	}
+	if len(parts) >= 2 {
+		ver.Minor, _ = strconv.Atoi(parts[1])
+	}
+	if len(parts) >= 3 {
+		ver.Patch, _ = strconv.Atoi(parts[2])
+	}
+
+	// Get prerelease if any; semver.Prerelease includes the leading hyphen, strip it
+	ver.Pre = strings.TrimPrefix(semver.Prerelease(v), "-")
+
+	return ver
+}
+
+// IsPrecise returns true if the version has explicit minor and patch components.
+// For example, "v6.0.0" is precise, but "v6" is not.
+func (v *SemanticVersion) IsPrecise() bool {
+	versionPart := strings.TrimPrefix(v.Raw, "v")
+	dotCount := strings.Count(versionPart, ".")
+	return dotCount >= 2 // Require at least major.minor.patch
+}
+
+// IsNewer returns true if v is strictly newer than other.
+// Uses golang.org/x/mod/semver.Compare for proper semantic version comparison.
+func (v *SemanticVersion) IsNewer(other *SemanticVersion) bool {
+	v1 := EnsureVPrefix(v.Raw)
+	v2 := EnsureVPrefix(other.Raw)
+	isNewer := semver.Compare(v1, v2) > 0
+	log.Printf("Version comparison: %s vs %s, isNewer=%v", v.Raw, other.Raw, isNewer)
+	return isNewer
+}
+
+// Compare compares two semantic versions and returns 1 if v1 > v2, -1 if v1 < v2,
+// or 0 if they are equal. A bare version without a leading "v" is accepted.
+func Compare(v1, v2 string) int {
+	v1 = EnsureVPrefix(v1)
+	v2 = EnsureVPrefix(v2)
+
+	result := semver.Compare(v1, v2)
+
+	if result > 0 {
+		log.Printf("Version comparison result: %s > %s", v1, v2)
+	} else if result < 0 {
+		log.Printf("Version comparison result: %s < %s", v1, v2)
+	} else {
+		log.Printf("Version comparison result: %s == %s", v1, v2)
+	}
+
+	return result
+}
+
+// IsCompatible reports whether pinVersion is semver-compatible with requestedVersion.
+// Semver compatibility is defined as both versions sharing the same major version.
+//
+// Examples:
+//   - IsCompatible("v5.0.0", "v5")    → true
+//   - IsCompatible("v5.1.0", "v5.0.0") → true
+//   - IsCompatible("v6.0.0", "v5")    → false
+func IsCompatible(pinVersion, requestedVersion string) bool {
+	pinVersion = EnsureVPrefix(pinVersion)
+	requestedVersion = EnsureVPrefix(requestedVersion)
+
+	pinMajor := semver.Major(pinVersion)
+	requestedMajor := semver.Major(requestedVersion)
+
+	compatible := pinMajor == requestedMajor
+	log.Printf("Checking semver compatibility: pin=%s (major=%s), requested=%s (major=%s) -> %v",
+		pinVersion, pinMajor, requestedVersion, requestedMajor, compatible)
+
+	return compatible
+}