From 1c9325f9fdfe73e64a01bc158b7637a34de2545f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 25 Jan 2026 15:54:48 +0000 Subject: [PATCH 1/5] Initial plan From 6acff1a11a0452c80640f3b5161e78d5eb6d0c94 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 25 Jan 2026 16:05:45 +0000 Subject: [PATCH 2/5] Phase 1 refactoring: Add validation helpers, expression patterns, rename files for consistency Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/dispatch_workflow_validation.go | 6 - pkg/workflow/expression_patterns.go | 144 ++++++++++++ ...onfig-builtin.go => mcp_config_builtin.go} | 0 ...-config-custom.go => mcp_config_custom.go} | 0 ...t.go => mcp_config_playwright_renderer.go} | 2 +- ...erena.go => mcp_config_serena_renderer.go} | 2 +- ...cp-config-types.go => mcp_config_types.go} | 0 ...cp-config-utils.go => mcp_config_utils.go} | 0 ...validator.go => permissions_validation.go} | 26 +-- pkg/workflow/validation_helpers.go | 212 ++++++++++++++++++ 10 files changed, 371 insertions(+), 21 deletions(-) create mode 100644 pkg/workflow/expression_patterns.go rename pkg/workflow/{mcp-config-builtin.go => mcp_config_builtin.go} (100%) rename pkg/workflow/{mcp-config-custom.go => mcp_config_custom.go} (100%) rename pkg/workflow/{mcp-config-playwright.go => mcp_config_playwright_renderer.go} (98%) rename pkg/workflow/{mcp-config-serena.go => mcp_config_serena_renderer.go} (98%) rename pkg/workflow/{mcp-config-types.go => mcp_config_types.go} (100%) rename pkg/workflow/{mcp-config-utils.go => mcp_config_utils.go} (100%) rename pkg/workflow/{permissions_validator.go => permissions_validation.go} (89%) diff --git a/pkg/workflow/dispatch_workflow_validation.go b/pkg/workflow/dispatch_workflow_validation.go index aa2869cebd..7724bfbf94 100644 --- a/pkg/workflow/dispatch_workflow_validation.go +++ b/pkg/workflow/dispatch_workflow_validation.go @@ -191,12 +191,6 @@ func getCurrentWorkflowName(workflowPath string) string { return filename } -// fileExists checks if a file exists -func fileExists(path string) bool { - _, err := os.Stat(path) - return err == nil -} - // isPathWithinDir checks if a path is within a given directory (prevents path traversal) func isPathWithinDir(path, dir string) bool { // Get absolute paths diff --git a/pkg/workflow/expression_patterns.go b/pkg/workflow/expression_patterns.go new file mode 100644 index 0000000000..44d3ae0619 --- /dev/null +++ b/pkg/workflow/expression_patterns.go @@ -0,0 +1,144 @@ +// Package workflow provides centralized regex patterns for GitHub Actions expression matching. +// +// # Expression Patterns +// +// This file consolidates regular expression patterns used across multiple validation and +// extraction files to provide a single source of truth for expression matching logic. +// +// # Available Pattern Categories +// +// ## Core Expression Patterns +// - ExpressionPattern - Matches GitHub Actions expressions: ${{ ... }} +// - ExpressionPatternDotAll - Matches expressions with dotall mode (multiline) +// +// ## Context Access Patterns +// - NeedsStepsPattern - Matches needs.* and steps.* patterns +// - InputsPattern - Matches github.event.inputs.* patterns +// - WorkflowCallInputsPattern - Matches inputs.* patterns (workflow_call) +// - AWInputsPattern - Matches github.aw.inputs.* patterns +// - EnvPattern - Matches env.* patterns +// +// ## Secret Patterns +// - SecretExpressionPattern - Matches ${{ secrets.SECRET_NAME }} expressions +// - SecretsExpressionPattern - Validates secrets expression syntax +// +// ## Template Patterns +// - InlineExpressionPattern - Matches inline expressions in templates +// - UnsafeContextPattern - Matches potentially unsafe context patterns +// - TemplateIfPattern - Matches {{#if ...}} template conditionals +// +// ## Utility Patterns +// - ComparisonExtractionPattern - Extracts properties from comparison expressions +// - StringLiteralPattern - Matches string literals ('...', "...", `...`) +// - NumberLiteralPattern - Matches numeric literals +// - RangePattern - Matches numeric ranges (e.g., "1-10") +// +// # Design Rationale +// +// Centralizing regex patterns provides several benefits: +// - Single source of truth for expression matching logic +// - Consistent behavior across validation and extraction +// - Easier to maintain and update patterns +// - Better performance through pre-compilation +// - Reduced code duplication across files +// +// # Migration Notes +// +// This file consolidates patterns previously scattered across: +// - expression_validation.go +// - expression_extraction.go +// - secret_extraction.go +// - secrets_validation.go +// - template.go +// - template_injection_validation.go +// +// Files are gradually being migrated to use these centralized patterns. +package workflow + +import "regexp" + +// Core Expression Patterns +var ( + // ExpressionPattern matches GitHub Actions expressions: ${{ ... }} + // Uses non-greedy matching to handle nested braces properly + ExpressionPattern = regexp.MustCompile(`\$\{\{(.*?)\}\}`) + + // ExpressionPatternDotAll matches expressions with dotall mode enabled + // The (?s) flag enables dotall mode where . matches newlines + ExpressionPatternDotAll = regexp.MustCompile(`(?s)\$\{\{(.*?)\}\}`) +) + +// Context Access Patterns +var ( + // NeedsStepsPattern matches needs.* and steps.* context patterns + // Example: needs.build.outputs.version, steps.setup.outputs.path + NeedsStepsPattern = regexp.MustCompile(`^(needs|steps)\.[a-zA-Z0-9_-]+(\.[a-zA-Z0-9_-]+)*$`) + + // InputsPattern matches github.event.inputs.* patterns + // Example: github.event.inputs.workflow_id + InputsPattern = regexp.MustCompile(`^github\.event\.inputs\.[a-zA-Z0-9_-]+$`) + + // WorkflowCallInputsPattern matches inputs.* patterns for workflow_call + // Example: inputs.branch_name + WorkflowCallInputsPattern = regexp.MustCompile(`^inputs\.[a-zA-Z0-9_-]+$`) + + // AWInputsPattern matches github.aw.inputs.* patterns + // Example: github.aw.inputs.custom_param + AWInputsPattern = regexp.MustCompile(`^github\.aw\.inputs\.[a-zA-Z0-9_-]+$`) + + // AWInputsExpressionPattern matches full ${{ github.aw.inputs.* }} expressions + // Used for extraction rather than validation + AWInputsExpressionPattern = regexp.MustCompile(`\$\{\{\s*github\.aw\.inputs\.([a-zA-Z0-9_-]+)\s*\}\}`) + + // EnvPattern matches env.* patterns + // Example: env.NODE_VERSION + EnvPattern = regexp.MustCompile(`^env\.[a-zA-Z0-9_-]+$`) +) + +// Secret Patterns +var ( + // SecretExpressionPattern matches ${{ secrets.SECRET_NAME }} expressions + // Captures the secret name and supports optional || fallback + SecretExpressionPattern = regexp.MustCompile(`\$\{\{\s*secrets\.([A-Z_][A-Z0-9_]*)\s*(?:\|\|.*?)?\s*\}\}`) + + // SecretsExpressionPattern validates complete secrets expression syntax + // Supports chained || fallbacks: ${{ secrets.A || secrets.B }} + SecretsExpressionPattern = regexp.MustCompile(`^\$\{\{\s*secrets\.[A-Za-z_][A-Za-z0-9_]*(\s*\|\|\s*secrets\.[A-Za-z_][A-Za-z0-9_]*)*\s*\}\}$`) +) + +// Template Patterns +var ( + // InlineExpressionPattern matches inline ${{ ... }} expressions in templates + InlineExpressionPattern = regexp.MustCompile(`\$\{\{[^}]+\}\}`) + + // UnsafeContextPattern matches potentially unsafe context patterns + // These patterns may allow injection attacks in templates + UnsafeContextPattern = regexp.MustCompile(`\$\{\{\s*(github\.event\.|steps\.[^}]+\.outputs\.|inputs\.)[^}]+\}\}`) + + // TemplateIfPattern matches {{#if condition }} template conditionals + // Captures the condition expression (which may contain ${{ ... }}) + TemplateIfPattern = regexp.MustCompile(`\{\{#if\s+((?:\$\{\{[^\}]*\}\}|[^\}])*)\s*\}\}`) +) + +// Comparison and Literal Patterns +var ( + // ComparisonExtractionPattern extracts property accesses from comparison expressions + // Matches patterns like "github.workflow == 'value'" and extracts "github.workflow" + ComparisonExtractionPattern = regexp.MustCompile(`([a-zA-Z_][a-zA-Z0-9_.]*)\s*(?:==|!=|<|>|<=|>=)\s*`) + + // OrPattern matches logical OR expressions + // Example: value1 || value2 + OrPattern = regexp.MustCompile(`^(.+?)\s*\|\|\s*(.+)$`) + + // StringLiteralPattern matches string literals in single quotes, double quotes, or backticks + // Example: 'hello', "world", `template` + StringLiteralPattern = regexp.MustCompile(`^'[^']*'$|^"[^"]*"$|^` + "`[^`]*`$") + + // NumberLiteralPattern matches numeric literals (integers and decimals) + // Example: 42, -3.14, 0.5 + NumberLiteralPattern = regexp.MustCompile(`^-?\d+(\.\d+)?$`) + + // RangePattern matches numeric range patterns + // Example: 1-10, 100-200 + RangePattern = regexp.MustCompile(`^\d+-\d+$`) +) diff --git a/pkg/workflow/mcp-config-builtin.go b/pkg/workflow/mcp_config_builtin.go similarity index 100% rename from pkg/workflow/mcp-config-builtin.go rename to pkg/workflow/mcp_config_builtin.go diff --git a/pkg/workflow/mcp-config-custom.go b/pkg/workflow/mcp_config_custom.go similarity index 100% rename from pkg/workflow/mcp-config-custom.go rename to pkg/workflow/mcp_config_custom.go diff --git a/pkg/workflow/mcp-config-playwright.go b/pkg/workflow/mcp_config_playwright_renderer.go similarity index 98% rename from pkg/workflow/mcp-config-playwright.go rename to pkg/workflow/mcp_config_playwright_renderer.go index 460c0779c5..ec59911f73 100644 --- a/pkg/workflow/mcp-config-playwright.go +++ b/pkg/workflow/mcp_config_playwright_renderer.go @@ -6,7 +6,7 @@ import ( "github.com/githubnext/gh-aw/pkg/logger" ) -var mcpPlaywrightLog = logger.New("workflow:mcp-config-playwright") +var mcpPlaywrightLog = logger.New("workflow:mcp_config_playwright_renderer") // renderPlaywrightMCPConfig generates the Playwright MCP server configuration // Uses Docker container to launch Playwright MCP for consistent browser environment diff --git a/pkg/workflow/mcp-config-serena.go b/pkg/workflow/mcp_config_serena_renderer.go similarity index 98% rename from pkg/workflow/mcp-config-serena.go rename to pkg/workflow/mcp_config_serena_renderer.go index 8270d8029e..5bc579940e 100644 --- a/pkg/workflow/mcp-config-serena.go +++ b/pkg/workflow/mcp_config_serena_renderer.go @@ -7,7 +7,7 @@ import ( "github.com/githubnext/gh-aw/pkg/logger" ) -var mcpSerenaLog = logger.New("workflow:mcp-config-serena") +var mcpSerenaLog = logger.New("workflow:mcp_config_serena_renderer") // selectSerenaContainer determines which Serena container image to use based on requested languages // Returns the container image path that supports all requested languages diff --git a/pkg/workflow/mcp-config-types.go b/pkg/workflow/mcp_config_types.go similarity index 100% rename from pkg/workflow/mcp-config-types.go rename to pkg/workflow/mcp_config_types.go diff --git a/pkg/workflow/mcp-config-utils.go b/pkg/workflow/mcp_config_utils.go similarity index 100% rename from pkg/workflow/mcp-config-utils.go rename to pkg/workflow/mcp_config_utils.go diff --git a/pkg/workflow/permissions_validator.go b/pkg/workflow/permissions_validation.go similarity index 89% rename from pkg/workflow/permissions_validator.go rename to pkg/workflow/permissions_validation.go index 9ec9b50fdc..453720de4f 100644 --- a/pkg/workflow/permissions_validator.go +++ b/pkg/workflow/permissions_validation.go @@ -10,7 +10,7 @@ import ( "github.com/githubnext/gh-aw/pkg/logger" ) -var permissionsValidatorLog = logger.New("workflow:permissions_validator") +var permissionsValidationLog = logger.New("workflow:permissions_validation") //go:embed data/github_toolsets_permissions.json var githubToolsetsPermissionsJSON []byte @@ -40,7 +40,7 @@ var toolsetPermissionsMap map[string]GitHubToolsetPermissions // init loads the GitHub toolsets and permissions from the embedded JSON func init() { - permissionsValidatorLog.Print("Loading GitHub toolsets permissions from embedded JSON") + permissionsValidationLog.Print("Loading GitHub toolsets permissions from embedded JSON") var data GitHubToolsetsData if err := json.Unmarshal(githubToolsetsPermissionsJSON, &data); err != nil { @@ -68,7 +68,7 @@ func init() { } } - permissionsValidatorLog.Printf("Loaded %d GitHub toolsets from JSON", len(toolsetPermissionsMap)) + permissionsValidationLog.Printf("Loaded %d GitHub toolsets from JSON", len(toolsetPermissionsMap)) } // GetToolsetsData returns the parsed GitHub toolsets data (for use by workflows) @@ -132,7 +132,7 @@ type PermissionsValidationResult struct { // Use ValidatePermissions (this function) for general permission validation against GitHub MCP toolsets. // Use ValidateIncludedPermissions (in imports.go) when validating permissions from included/imported workflow files. func ValidatePermissions(permissions *Permissions, githubTool ValidatableTool) *PermissionsValidationResult { - permissionsValidatorLog.Print("Starting permissions validation") + permissionsValidationLog.Print("Starting permissions validation") result := &PermissionsValidationResult{ MissingPermissions: make(map[PermissionScope]PermissionLevel), @@ -142,13 +142,13 @@ func ValidatePermissions(permissions *Permissions, githubTool ValidatableTool) * // If GitHub tool is not configured, no validation needed // Check both for nil interface and nil concrete type if githubTool == nil { - permissionsValidatorLog.Print("No GitHub tool configured (nil interface), skipping validation") + permissionsValidationLog.Print("No GitHub tool configured (nil interface), skipping validation") return result } // Check if concrete type is nil (interface wrapping nil pointer) if config, ok := githubTool.(*GitHubToolConfig); ok && config == nil { - permissionsValidatorLog.Print("No GitHub tool configured (nil concrete type), skipping validation") + permissionsValidationLog.Print("No GitHub tool configured (nil concrete type), skipping validation") return result } @@ -157,37 +157,37 @@ func ValidatePermissions(permissions *Permissions, githubTool ValidatableTool) * readOnly := githubTool.IsReadOnly() result.ReadOnlyMode = readOnly - permissionsValidatorLog.Printf("Validating toolsets: %s, read-only: %v", toolsetsStr, readOnly) + permissionsValidationLog.Printf("Validating toolsets: %s, read-only: %v", toolsetsStr, readOnly) // Parse toolsets toolsets := ParseGitHubToolsets(toolsetsStr) if len(toolsets) == 0 { - permissionsValidatorLog.Print("No toolsets to validate") + permissionsValidationLog.Print("No toolsets to validate") return result } // Collect required permissions for all toolsets requiredPermissions := collectRequiredPermissions(toolsets, readOnly) - permissionsValidatorLog.Printf("Required permissions: %v", requiredPermissions) + permissionsValidationLog.Printf("Required permissions: %v", requiredPermissions) // Check for missing permissions checkMissingPermissions(permissions, requiredPermissions, toolsets, result) result.HasValidationIssues = len(result.MissingPermissions) > 0 - permissionsValidatorLog.Printf("Validation complete: hasIssues=%v, missingCount=%d", result.HasValidationIssues, len(result.MissingPermissions)) + permissionsValidationLog.Printf("Validation complete: hasIssues=%v, missingCount=%d", result.HasValidationIssues, len(result.MissingPermissions)) return result } // collectRequiredPermissions collects all required permissions for the given toolsets func collectRequiredPermissions(toolsets []string, readOnly bool) map[PermissionScope]PermissionLevel { - permissionsValidatorLog.Printf("Collecting required permissions for %d toolsets, read_only=%t", len(toolsets), readOnly) + permissionsValidationLog.Printf("Collecting required permissions for %d toolsets, read_only=%t", len(toolsets), readOnly) required := make(map[PermissionScope]PermissionLevel) for _, toolset := range toolsets { perms, exists := toolsetPermissionsMap[toolset] if !exists { - permissionsValidatorLog.Printf("Unknown toolset: %s", toolset) + permissionsValidationLog.Printf("Unknown toolset: %s", toolset) continue } @@ -212,7 +212,7 @@ func collectRequiredPermissions(toolsets []string, readOnly bool) map[Permission // checkMissingPermissions checks if all required permissions are granted func checkMissingPermissions(permissions *Permissions, required map[PermissionScope]PermissionLevel, toolsets []string, result *PermissionsValidationResult) { - permissionsValidatorLog.Printf("Checking missing permissions: required_count=%d, toolsets=%v", len(required), toolsets) + permissionsValidationLog.Printf("Checking missing permissions: required_count=%d, toolsets=%v", len(required), toolsets) for scope, requiredLevel := range required { grantedLevel, granted := permissions.Get(scope) diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index efcad4f4ff..60fb8cf52a 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -4,11 +4,38 @@ // such as integer range validation, string validation, and list membership checks. // These utilities are used across multiple workflow configuration validation functions. // +// # Available Helper Functions +// +// - validateIntRange() - Validates that an integer value is within a specified range +// - ValidateRequired() - Validates that a required field is not empty +// - ValidateMaxLength() - Validates that a field does not exceed maximum length +// - ValidateMinLength() - Validates that a field meets minimum length requirement +// - ValidateInList() - Validates that a value is in an allowed list +// - ValidatePositiveInt() - Validates that a value is a positive integer +// - ValidateNonNegativeInt() - Validates that a value is a non-negative integer +// - isEmptyOrNil() - Checks if a value is empty, nil, or zero +// - getMapFieldAsString() - Safely extracts a string field from a map[string]any +// - getMapFieldAsMap() - Safely extracts a nested map from a map[string]any +// - getMapFieldAsBool() - Safely extracts a boolean field from a map[string]any +// - getMapFieldAsInt() - Safely extracts an integer field from a map[string]any +// - fileExists() - Checks if a file exists at the given path +// - dirExists() - Checks if a directory exists at the given path +// +// # Design Rationale +// +// These helpers consolidate 76+ duplicate validation patterns identified in the +// semantic function clustering analysis. By extracting common patterns, we: +// - Reduce code duplication across 32 validation files +// - Provide consistent validation behavior +// - Make validation code more maintainable and testable +// - Reduce cognitive overhead when writing new validators +// // For the validation architecture overview, see validation.go. package workflow import ( "fmt" + "os" "strings" ) @@ -120,3 +147,188 @@ func ValidateNonNegativeInt(field string, value int) error { } return nil } + +// isEmptyOrNil checks if a value is considered empty or nil. +// Returns true if the value is nil, an empty string, zero, or false. +// +// This helper consolidates common empty-checking patterns across validation files. +// +// Example: +// +// if isEmptyOrNil(config.Name) { +// return NewValidationError("config.name", "", "name cannot be empty", "Provide a valid name") +// } +func isEmptyOrNil(value any) bool { + if value == nil { + return true + } + + switch v := value.(type) { + case string: + return v == "" + case int, int32, int64: + return v == 0 + case bool: + return !v + case []string: + return len(v) == 0 + case []any: + return len(v) == 0 + case map[string]any: + return len(v) == 0 + default: + return false + } +} + +// getMapFieldAsString safely extracts a string field from a map[string]any. +// Returns the string value and a boolean indicating success. +// +// This helper consolidates common type assertion patterns for string fields. +// +// Example: +// +// if name, ok := getMapFieldAsString(config, "name"); ok { +// // Use name +// } +func getMapFieldAsString(m map[string]any, key string) (string, bool) { + if m == nil { + return "", false + } + + value, exists := m[key] + if !exists { + return "", false + } + + str, ok := value.(string) + return str, ok +} + +// getMapFieldAsMap safely extracts a nested map from a map[string]any. +// Returns the nested map and a boolean indicating success. +// +// This helper consolidates common type assertion patterns for nested maps. +// +// Example: +// +// if config, ok := getMapFieldAsMap(frontmatter, "tools"); ok { +// // Process tools config +// } +func getMapFieldAsMap(m map[string]any, key string) (map[string]any, bool) { + if m == nil { + return nil, false + } + + value, exists := m[key] + if !exists { + return nil, false + } + + nested, ok := value.(map[string]any) + return nested, ok +} + +// getMapFieldAsBool safely extracts a boolean field from a map[string]any. +// Returns the boolean value and a boolean indicating success. +// +// This helper consolidates common type assertion patterns for boolean fields. +// +// Example: +// +// if enabled, ok := getMapFieldAsBool(config, "enabled"); ok && enabled { +// // Feature is enabled +// } +func getMapFieldAsBool(m map[string]any, key string) (bool, bool) { + if m == nil { + return false, false + } + + value, exists := m[key] + if !exists { + return false, false + } + + b, ok := value.(bool) + return b, ok +} + +// getMapFieldAsInt safely extracts an integer field from a map[string]any. +// Returns the integer value and a boolean indicating success. +// Handles both int and float64 types (common in JSON unmarshaling). +// +// This helper consolidates common type assertion patterns for integer fields. +// +// Example: +// +// if count, ok := getMapFieldAsInt(config, "max_count"); ok { +// // Use count +// } +func getMapFieldAsInt(m map[string]any, key string) (int, bool) { + if m == nil { + return 0, false + } + + value, exists := m[key] + if !exists { + return 0, false + } + + // Handle int type + if i, ok := value.(int); ok { + return i, true + } + + // Handle float64 (common in JSON unmarshaling) + if f, ok := value.(float64); ok { + return int(f), true + } + + return 0, false +} + +// fileExists checks if a file exists at the given path. +// Returns true if the file exists and is accessible, false otherwise. +// +// This helper consolidates common file existence checking patterns. +// +// Example: +// +// if !fileExists(agentPath) { +// return NewValidationError("agent.file", agentPath, "file does not exist", "...") +// } +func fileExists(path string) bool { + if path == "" { + return false + } + + info, err := os.Stat(path) + if err != nil { + return false + } + + return !info.IsDir() +} + +// dirExists checks if a directory exists at the given path. +// Returns true if the directory exists and is accessible, false otherwise. +// +// This helper is useful for validating directory paths in configuration. +// +// Example: +// +// if !dirExists(workspaceDir) { +// return NewValidationError("workspace", workspaceDir, "directory does not exist", "...") +// } +func dirExists(path string) bool { + if path == "" { + return false + } + + info, err := os.Stat(path) + if err != nil { + return false + } + + return info.IsDir() +} From 19369ecb4dc3a6fe93bae0d61d54248908749c00 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 25 Jan 2026 16:11:41 +0000 Subject: [PATCH 3/5] Update validation_helpers to keep only used functions, document future helpers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/validation_helpers.go | 169 ++--------------------------- 1 file changed, 8 insertions(+), 161 deletions(-) diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index 60fb8cf52a..e4a2098d62 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -148,145 +148,6 @@ func ValidateNonNegativeInt(field string, value int) error { return nil } -// isEmptyOrNil checks if a value is considered empty or nil. -// Returns true if the value is nil, an empty string, zero, or false. -// -// This helper consolidates common empty-checking patterns across validation files. -// -// Example: -// -// if isEmptyOrNil(config.Name) { -// return NewValidationError("config.name", "", "name cannot be empty", "Provide a valid name") -// } -func isEmptyOrNil(value any) bool { - if value == nil { - return true - } - - switch v := value.(type) { - case string: - return v == "" - case int, int32, int64: - return v == 0 - case bool: - return !v - case []string: - return len(v) == 0 - case []any: - return len(v) == 0 - case map[string]any: - return len(v) == 0 - default: - return false - } -} - -// getMapFieldAsString safely extracts a string field from a map[string]any. -// Returns the string value and a boolean indicating success. -// -// This helper consolidates common type assertion patterns for string fields. -// -// Example: -// -// if name, ok := getMapFieldAsString(config, "name"); ok { -// // Use name -// } -func getMapFieldAsString(m map[string]any, key string) (string, bool) { - if m == nil { - return "", false - } - - value, exists := m[key] - if !exists { - return "", false - } - - str, ok := value.(string) - return str, ok -} - -// getMapFieldAsMap safely extracts a nested map from a map[string]any. -// Returns the nested map and a boolean indicating success. -// -// This helper consolidates common type assertion patterns for nested maps. -// -// Example: -// -// if config, ok := getMapFieldAsMap(frontmatter, "tools"); ok { -// // Process tools config -// } -func getMapFieldAsMap(m map[string]any, key string) (map[string]any, bool) { - if m == nil { - return nil, false - } - - value, exists := m[key] - if !exists { - return nil, false - } - - nested, ok := value.(map[string]any) - return nested, ok -} - -// getMapFieldAsBool safely extracts a boolean field from a map[string]any. -// Returns the boolean value and a boolean indicating success. -// -// This helper consolidates common type assertion patterns for boolean fields. -// -// Example: -// -// if enabled, ok := getMapFieldAsBool(config, "enabled"); ok && enabled { -// // Feature is enabled -// } -func getMapFieldAsBool(m map[string]any, key string) (bool, bool) { - if m == nil { - return false, false - } - - value, exists := m[key] - if !exists { - return false, false - } - - b, ok := value.(bool) - return b, ok -} - -// getMapFieldAsInt safely extracts an integer field from a map[string]any. -// Returns the integer value and a boolean indicating success. -// Handles both int and float64 types (common in JSON unmarshaling). -// -// This helper consolidates common type assertion patterns for integer fields. -// -// Example: -// -// if count, ok := getMapFieldAsInt(config, "max_count"); ok { -// // Use count -// } -func getMapFieldAsInt(m map[string]any, key string) (int, bool) { - if m == nil { - return 0, false - } - - value, exists := m[key] - if !exists { - return 0, false - } - - // Handle int type - if i, ok := value.(int); ok { - return i, true - } - - // Handle float64 (common in JSON unmarshaling) - if f, ok := value.(float64); ok { - return int(f), true - } - - return 0, false -} - // fileExists checks if a file exists at the given path. // Returns true if the file exists and is accessible, false otherwise. // @@ -310,25 +171,11 @@ func fileExists(path string) bool { return !info.IsDir() } -// dirExists checks if a directory exists at the given path. -// Returns true if the directory exists and is accessible, false otherwise. -// -// This helper is useful for validating directory paths in configuration. -// -// Example: -// -// if !dirExists(workspaceDir) { -// return NewValidationError("workspace", workspaceDir, "directory does not exist", "...") -// } -func dirExists(path string) bool { - if path == "" { - return false - } - - info, err := os.Stat(path) - if err != nil { - return false - } - - return info.IsDir() -} +// The following helper functions are planned for Phase 2 refactoring and will +// consolidate 70+ duplicate validation patterns identified in the semantic analysis: +// - isEmptyOrNil() - Check if a value is empty, nil, or zero +// - getMapFieldAsString() - Safely extract a string field from a map[string]any +// - getMapFieldAsMap() - Safely extract a nested map from a map[string]any +// - getMapFieldAsBool() - Safely extract a boolean field from a map[string]any +// - getMapFieldAsInt() - Safely extract an integer field from a map[string]any +// - dirExists() - Check if a directory exists at the given path From 24696dfd1082b0c12238f02bea7b4f1fd9c85dc6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 25 Jan 2026 16:13:44 +0000 Subject: [PATCH 4/5] Add comprehensive tests for expression_patterns.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/expression_patterns_test.go | 460 +++++++++++++++++++++++ 1 file changed, 460 insertions(+) create mode 100644 pkg/workflow/expression_patterns_test.go diff --git a/pkg/workflow/expression_patterns_test.go b/pkg/workflow/expression_patterns_test.go new file mode 100644 index 0000000000..2084526b85 --- /dev/null +++ b/pkg/workflow/expression_patterns_test.go @@ -0,0 +1,460 @@ +// Package workflow provides tests for expression pattern matching. +package workflow + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestExpressionPattern tests the basic expression matching pattern +func TestExpressionPattern(t *testing.T) { + tests := []struct { + name string + input string + wantMatch bool + wantValue string + }{ + { + name: "simple expression", + input: "${{ github.actor }}", + wantMatch: true, + wantValue: " github.actor ", + }, + { + name: "expression with property", + input: "${{ github.event.inputs.branch }}", + wantMatch: true, + wantValue: " github.event.inputs.branch ", + }, + { + name: "expression with comparison", + input: "${{ github.workflow == 'CI' }}", + wantMatch: true, + wantValue: " github.workflow == 'CI' ", + }, + { + name: "no expression", + input: "plain text", + wantMatch: false, + }, + { + name: "multiple expressions (matches first)", + input: "${{ github.actor }} and ${{ github.repository }}", + wantMatch: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matches := ExpressionPattern.FindStringSubmatch(tt.input) + if tt.wantMatch { + assert.NotNil(t, matches, "Expected to find expression match") + if len(matches) > 1 && tt.wantValue != "" { + assert.Equal(t, tt.wantValue, matches[1], "Expression content mismatch") + } + } else { + assert.Nil(t, matches, "Expected no expression match") + } + }) + } +} + +// TestNeedsStepsPattern tests the needs/steps context pattern +func TestNeedsStepsPattern(t *testing.T) { + tests := []struct { + name string + input string + wantMatch bool + }{ + { + name: "needs with output", + input: "needs.build.outputs.version", + wantMatch: true, + }, + { + name: "steps with output", + input: "steps.setup.outputs.path", + wantMatch: true, + }, + { + name: "needs with result", + input: "needs.test.result", + wantMatch: true, + }, + { + name: "steps with conclusion", + input: "steps.deploy.conclusion", + wantMatch: true, + }, + { + name: "invalid - github prefix", + input: "github.event.inputs.branch", + wantMatch: false, + }, + { + name: "invalid - no job/step name", + input: "needs.outputs.version", + wantMatch: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matches := NeedsStepsPattern.MatchString(tt.input) + assert.Equal(t, tt.wantMatch, matches, "NeedsStepsPattern match result mismatch") + }) + } +} + +// TestInputsPattern tests the github.event.inputs pattern +func TestInputsPattern(t *testing.T) { + tests := []struct { + name string + input string + wantMatch bool + }{ + { + name: "valid workflow_dispatch input", + input: "github.event.inputs.branch", + wantMatch: true, + }, + { + name: "valid input with hyphen", + input: "github.event.inputs.workflow-id", + wantMatch: true, + }, + { + name: "valid input with underscore", + input: "github.event.inputs.some_param", + wantMatch: true, + }, + { + name: "invalid - no input name", + input: "github.event.inputs", + wantMatch: false, + }, + { + name: "invalid - nested property", + input: "github.event.inputs.branch.name", + wantMatch: false, + }, + { + name: "invalid - wrong prefix", + input: "inputs.branch", + wantMatch: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matches := InputsPattern.MatchString(tt.input) + assert.Equal(t, tt.wantMatch, matches, "InputsPattern match result mismatch") + }) + } +} + +// TestWorkflowCallInputsPattern tests the workflow_call inputs pattern +func TestWorkflowCallInputsPattern(t *testing.T) { + tests := []struct { + name string + input string + wantMatch bool + }{ + { + name: "valid workflow_call input", + input: "inputs.branch", + wantMatch: true, + }, + { + name: "valid input with hyphen", + input: "inputs.workflow-id", + wantMatch: true, + }, + { + name: "valid input with underscore", + input: "inputs.some_param", + wantMatch: true, + }, + { + name: "invalid - no input name", + input: "inputs", + wantMatch: false, + }, + { + name: "invalid - nested property", + input: "inputs.branch.name", + wantMatch: false, + }, + { + name: "invalid - github prefix", + input: "github.event.inputs.branch", + wantMatch: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matches := WorkflowCallInputsPattern.MatchString(tt.input) + assert.Equal(t, tt.wantMatch, matches, "WorkflowCallInputsPattern match result mismatch") + }) + } +} + +// TestSecretExpressionPattern tests the secrets expression pattern +func TestSecretExpressionPattern(t *testing.T) { + tests := []struct { + name string + input string + wantMatch bool + wantSecretName string + }{ + { + name: "simple secret", + input: "${{ secrets.GITHUB_TOKEN }}", + wantMatch: true, + wantSecretName: "GITHUB_TOKEN", + }, + { + name: "secret with fallback", + input: "${{ secrets.MY_TOKEN || 'default' }}", + wantMatch: true, + wantSecretName: "MY_TOKEN", + }, + { + name: "secret with underscore prefix", + input: "${{ secrets._INTERNAL_SECRET }}", + wantMatch: true, + wantSecretName: "_INTERNAL_SECRET", + }, + { + name: "invalid - lowercase secret", + input: "${{ secrets.my_token }}", + wantMatch: false, + }, + { + name: "invalid - starts with number", + input: "${{ secrets.123_TOKEN }}", + wantMatch: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matches := SecretExpressionPattern.FindStringSubmatch(tt.input) + if tt.wantMatch { + assert.NotNil(t, matches, "Expected to find secret match") + if len(matches) > 1 && tt.wantSecretName != "" { + assert.Equal(t, tt.wantSecretName, matches[1], "Secret name mismatch") + } + } else { + assert.Nil(t, matches, "Expected no secret match") + } + }) + } +} + +// TestComparisonExtractionPattern tests the comparison extraction pattern +func TestComparisonExtractionPattern(t *testing.T) { + tests := []struct { + name string + input string + wantMatch bool + wantProperty string + }{ + { + name: "equality comparison", + input: "github.workflow == 'CI'", + wantMatch: true, + wantProperty: "github.workflow", + }, + { + name: "inequality comparison", + input: "github.actor != 'bot'", + wantMatch: true, + wantProperty: "github.actor", + }, + { + name: "less than comparison", + input: "count < 10", + wantMatch: true, + wantProperty: "count", + }, + { + name: "greater than or equal", + input: "value >= 5", + wantMatch: true, + wantProperty: "value", + }, + { + name: "no comparison operator", + input: "github.workflow", + wantMatch: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matches := ComparisonExtractionPattern.FindStringSubmatch(tt.input) + if tt.wantMatch { + assert.NotNil(t, matches, "Expected to find comparison match") + if len(matches) > 1 && tt.wantProperty != "" { + assert.Equal(t, tt.wantProperty, matches[1], "Property name mismatch") + } + } else { + assert.Nil(t, matches, "Expected no comparison match") + } + }) + } +} + +// TestStringLiteralPattern tests string literal matching +func TestStringLiteralPattern(t *testing.T) { + tests := []struct { + name string + input string + wantMatch bool + }{ + { + name: "single quoted string", + input: "'hello world'", + wantMatch: true, + }, + { + name: "double quoted string", + input: `"hello world"`, + wantMatch: true, + }, + { + name: "backtick quoted string", + input: "`hello world`", + wantMatch: true, + }, + { + name: "empty single quotes", + input: "''", + wantMatch: true, + }, + { + name: "not a string literal", + input: "hello world", + wantMatch: false, + }, + { + name: "mismatched quotes", + input: "'hello\"", + wantMatch: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matches := StringLiteralPattern.MatchString(tt.input) + assert.Equal(t, tt.wantMatch, matches, "StringLiteralPattern match result mismatch") + }) + } +} + +// TestNumberLiteralPattern tests numeric literal matching +func TestNumberLiteralPattern(t *testing.T) { + tests := []struct { + name string + input string + wantMatch bool + }{ + { + name: "positive integer", + input: "42", + wantMatch: true, + }, + { + name: "negative integer", + input: "-42", + wantMatch: true, + }, + { + name: "positive decimal", + input: "3.14", + wantMatch: true, + }, + { + name: "negative decimal", + input: "-3.14", + wantMatch: true, + }, + { + name: "zero", + input: "0", + wantMatch: true, + }, + { + name: "decimal with leading zero", + input: "0.5", + wantMatch: true, + }, + { + name: "not a number", + input: "abc", + wantMatch: false, + }, + { + name: "number with text", + input: "42abc", + wantMatch: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matches := NumberLiteralPattern.MatchString(tt.input) + assert.Equal(t, tt.wantMatch, matches, "NumberLiteralPattern match result mismatch") + }) + } +} + +// TestRangePattern tests numeric range pattern matching +func TestRangePattern(t *testing.T) { + tests := []struct { + name string + input string + wantMatch bool + }{ + { + name: "valid range", + input: "1-10", + wantMatch: true, + }, + { + name: "large range", + input: "100-200", + wantMatch: true, + }, + { + name: "single digit range", + input: "0-9", + wantMatch: true, + }, + { + name: "invalid - no hyphen", + input: "10", + wantMatch: false, + }, + { + name: "invalid - negative numbers", + input: "-1-10", + wantMatch: false, + }, + { + name: "invalid - text", + input: "one-ten", + wantMatch: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + matches := RangePattern.MatchString(tt.input) + assert.Equal(t, tt.wantMatch, matches, "RangePattern match result mismatch") + }) + } +} From 2e19cb0f333019ca4d6b45e530ebaed48bd6e062 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 25 Jan 2026 19:45:14 +0000 Subject: [PATCH 5/5] Merge main, format code, and lint Go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/expression_patterns_test.go | 28 ++++++++++++------------ 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/pkg/workflow/expression_patterns_test.go b/pkg/workflow/expression_patterns_test.go index 2084526b85..84bb0ffc88 100644 --- a/pkg/workflow/expression_patterns_test.go +++ b/pkg/workflow/expression_patterns_test.go @@ -10,8 +10,8 @@ import ( // TestExpressionPattern tests the basic expression matching pattern func TestExpressionPattern(t *testing.T) { tests := []struct { - name string - input string + name string + input string wantMatch bool wantValue string }{ @@ -204,27 +204,27 @@ func TestWorkflowCallInputsPattern(t *testing.T) { // TestSecretExpressionPattern tests the secrets expression pattern func TestSecretExpressionPattern(t *testing.T) { tests := []struct { - name string - input string - wantMatch bool + name string + input string + wantMatch bool wantSecretName string }{ { - name: "simple secret", - input: "${{ secrets.GITHUB_TOKEN }}", - wantMatch: true, + name: "simple secret", + input: "${{ secrets.GITHUB_TOKEN }}", + wantMatch: true, wantSecretName: "GITHUB_TOKEN", }, { - name: "secret with fallback", - input: "${{ secrets.MY_TOKEN || 'default' }}", - wantMatch: true, + name: "secret with fallback", + input: "${{ secrets.MY_TOKEN || 'default' }}", + wantMatch: true, wantSecretName: "MY_TOKEN", }, { - name: "secret with underscore prefix", - input: "${{ secrets._INTERNAL_SECRET }}", - wantMatch: true, + name: "secret with underscore prefix", + input: "${{ secrets._INTERNAL_SECRET }}", + wantMatch: true, wantSecretName: "_INTERNAL_SECRET", }, {