refactor: Semantic function clustering — move validation to proper files, remove wrappers, generify duplicate-ID checks

pkg/cli/deps_report.go

@@ -35,10 +35,12 @@ func GenerateDependencyReport(verbose bool) (*DependencyReport, error) {
 	}
 
 	// Parse go.mod to get all dependencies
-	allDeps, err := parseGoModWithIndirect(goModPath)
+	depsReportLog.Printf("Parsing go.mod file: %s", goModPath)
+	allDeps, err := parseGoModFile(goModPath)
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse go.mod: %w", err)
 	}
+	depsReportLog.Printf("Parsed go.mod: %d total dependencies", len(allDeps))
 
 	// Count direct vs indirect dependencies
 	directCount := 0
@@ -270,18 +272,6 @@ type DependencyInfoWithIndirect struct {
 	Indirect bool
 }
 
-// parseGoModWithIndirect parses go.mod including indirect dependencies.
-// This is a thin wrapper around parseGoModFile for backward compatibility.
-func parseGoModWithIndirect(path string) ([]DependencyInfoWithIndirect, error) {
-	depsReportLog.Printf("Parsing go.mod file: %s", path)
-	deps, err := parseGoModFile(path)
-	if err != nil {
-		return nil, err
-	}
-	depsReportLog.Printf("Parsed go.mod: %d total dependencies", len(deps))
-	return deps, nil
-}
-
 // pluralize returns the singular or plural form of a word based on count
 func pluralize(word string, count int) string {
 	if count == 1 {

pkg/cli/deps_test.go

@@ -133,7 +133,7 @@ require (
 	}
 }
 
-func TestParseGoModWithIndirect(t *testing.T) {
+func TestParseGoModFile_WithIndirect(t *testing.T) {
 	goModContent := `module github.com/example/test
 
 go 1.25.0
@@ -148,14 +148,14 @@ require (
 	tmpFile := createTempFile(t, goModContent)
 	defer removeTempFile(t, tmpFile)
 
-	deps, err := parseGoModWithIndirect(tmpFile)
+	deps, err := parseGoModFile(tmpFile)
 	if err != nil {
-		t.Fatalf("parseGoModWithIndirect() error = %v", err)
+		t.Fatalf("parseGoModFile() error = %v", err)
 	}
 
 	// Should parse all dependencies including indirect
 	if len(deps) != 3 {
-		t.Errorf("parseGoModWithIndirect() found %d dependencies, want 3", len(deps))
+		t.Errorf("parseGoModFile() found %d dependencies, want 3", len(deps))
 	}
 
 	// Check indirect flag
@@ -166,7 +166,7 @@ require (
 		}
 	}
 	if indirectCount != 1 {
-		t.Errorf("parseGoModWithIndirect() found %d indirect dependencies, want 1", indirectCount)
+		t.Errorf("parseGoModFile() found %d indirect dependencies, want 1", indirectCount)
 	}
 }
 

pkg/cli/mcp_server_helpers.go

@@ -126,9 +126,13 @@ func hasWriteAccess(permission string) bool {
 	}
 }
 
-// validateWorkflowName validates that a workflow name exists.
+// validateWorkflowName 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).
+//
+// 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
 	if workflowName == "" {

pkg/cli/pr_command.go

@@ -103,11 +103,6 @@ The command will:
 	return cmd
 }
 
-// parsePRURL extracts owner, repo, and PR number from a GitHub PR URL
-func parsePRURL(prURL string) (owner, repo string, prNumber int, err error) {
-	return parser.ParsePRURL(prURL)
-}
-
 // checkRepositoryAccess checks if the current user has write access to the target repository
 func checkRepositoryAccess(owner, repo string) (bool, error) {
 	prLog.Printf("Checking repository access: %s/%s", owner, repo)
@@ -548,7 +543,7 @@ func transferPR(prURL, targetRepo string, verbose bool) error {
 	}
 
 	// Parse PR URL
-	sourceOwner, sourceRepoName, prNumber, err := parsePRURL(prURL)
+	sourceOwner, sourceRepoName, prNumber, err := parser.ParsePRURL(prURL)
 	if err != nil {
 		prLog.Printf("Failed to parse PR URL: %s", err)
 		return err

pkg/cli/pr_command_test.go

@@ -4,6 +4,8 @@ package cli
 
 import (
 	"testing"
+
+	"github.com/github/gh-aw/pkg/parser"
 )
 
 func TestParsePRURL(t *testing.T) {
@@ -81,30 +83,30 @@ func TestParsePRURL(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			owner, repo, prNumber, err := parsePRURL(tt.url)
+			owner, repo, prNumber, err := parser.ParsePRURL(tt.url)
 
 			if tt.wantErr {
 				if err == nil {
-					t.Errorf("parsePRURL() expected error but got none")
+					t.Errorf("ParsePRURL() expected error but got none")
 				}
 				return
 			}
 
 			if err != nil {
-				t.Errorf("parsePRURL() unexpected error: %v", err)
+				t.Errorf("ParsePRURL() unexpected error: %v", err)
 				return
 			}
 
 			if owner != tt.wantOwner {
-				t.Errorf("parsePRURL() owner = %v, want %v", owner, tt.wantOwner)
+				t.Errorf("ParsePRURL() owner = %v, want %v", owner, tt.wantOwner)
 			}
 
 			if repo != tt.wantRepo {
-				t.Errorf("parsePRURL() repo = %v, want %v", repo, tt.wantRepo)
+				t.Errorf("ParsePRURL() repo = %v, want %v", repo, tt.wantRepo)
 			}
 
 			if prNumber != tt.wantPR {
-				t.Errorf("parsePRURL() prNumber = %v, want %v", prNumber, tt.wantPR)
+				t.Errorf("ParsePRURL() prNumber = %v, want %v", prNumber, tt.wantPR)
 			}
 		})
 	}

pkg/workflow/cache.go

@@ -864,23 +864,3 @@ func (c *Compiler) buildUpdateCacheMemoryJob(data *WorkflowData, threatDetection
 
 	return job, nil
 }
-
-// validateNoDuplicateCacheIDs checks for duplicate cache IDs and returns an error if found
-func validateNoDuplicateCacheIDs(caches []CacheMemoryEntry) error {
-	cacheLog.Printf("Validating cache IDs: checking %d caches for duplicates", len(caches))
-	seen := make(map[string]bool)
-	for _, cache := range caches {
-		if seen[cache.ID] {
-			cacheLog.Printf("Duplicate cache ID found: %s", cache.ID)
-			return NewValidationError(
-				"sandbox.cache-memory",
-				cache.ID,
-				"duplicate cache-memory ID found - each cache must have a unique ID",
-				"Change the cache ID to a unique value. Example:\n\nsandbox:\n  cache-memory:\n    - id: cache-1\n      size: 100MB\n    - id: cache-2  # Use unique IDs\n      size: 50MB",
-			)
-		}
-		seen[cache.ID] = true
-	}
-	cacheLog.Print("Cache ID validation passed: no duplicates found")
-	return nil
-}

pkg/workflow/cache_validation.go

@@ -0,0 +1,39 @@
+// This file provides validation for sandbox cache-memory configuration.
+//
+// # Cache Memory Validation
+//
+// This file validates that cache-memory entries in a workflow's sandbox
+// configuration have unique IDs, preventing runtime conflicts when multiple
+// cache entries are defined.
+//
+// # Validation Functions
+//
+//   - validateNoDuplicateCacheIDs() - Ensures each cache entry has a unique ID
+//
+// # When to Add Validation Here
+//
+// Add validation to this file when:
+//   - Adding new cache-memory configuration constraints
+//   - Adding cross-cache validation rules (e.g., total size limits)
+
+package workflow
+
+// validateNoDuplicateCacheIDs checks for duplicate cache IDs and returns an error if found.
+// Uses the generic validateNoDuplicateIDs helper for consistent duplicate detection.
+func validateNoDuplicateCacheIDs(caches []CacheMemoryEntry) error {
+	cacheLog.Printf("Validating cache IDs: checking %d caches for duplicates", len(caches))
+	err := validateNoDuplicateIDs(caches, func(c CacheMemoryEntry) string { return c.ID }, func(id string) error {
+		cacheLog.Printf("Duplicate cache ID found: %s", id)
+		return NewValidationError(
+			"sandbox.cache-memory",
+			id,
+			"duplicate cache-memory ID found - each cache must have a unique ID",
+			"Change the cache ID to a unique value. Example:\n\nsandbox:\n  cache-memory:\n    - id: cache-1\n      size: 100MB\n    - id: cache-2  # Use unique IDs\n      size: 50MB",
+		)
+	})
+	if err != nil {
+		return err
+	}
+	cacheLog.Print("Cache ID validation passed: no duplicates found")
+	return nil
+}