From 5b8ec73b871fc4747dfc76500ba1c42d86eb9eb1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 7 Feb 2026 08:08:08 +0000 Subject: [PATCH 1/3] Initial plan From 7f0a2451aa16c762743f10a0ccfe877d53889398 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 7 Feb 2026 08:15:42 +0000 Subject: [PATCH 2/3] Implement Phase 2 validation helpers (6 functions) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/validation_helpers.go | 269 ++++++++++++++++++- pkg/workflow/validation_helpers_test.go | 331 ++++++++++++++++++++++++ 2 files changed, 586 insertions(+), 14 deletions(-) diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index 00f7994eeee..c1cb5e8a7bd 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -13,13 +13,13 @@ // - 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 +// - isEmptyOrNil() - Checks if a value is empty, nil, or zero (Phase 2) +// - getMapFieldAsString() - Safely extracts a string field from a map[string]any (Phase 2) +// - getMapFieldAsMap() - Safely extracts a nested map from a map[string]any (Phase 2) +// - getMapFieldAsBool() - Safely extracts a boolean field from a map[string]any (Phase 2) +// - getMapFieldAsInt() - Safely extracts an integer field from a map[string]any (Phase 2) +// - dirExists() - Checks if a directory exists at the given path (Phase 2) // // # Design Rationale // @@ -180,11 +180,252 @@ func fileExists(path string) bool { 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 +// isEmptyOrNil evaluates whether a value represents an empty or absent state. +// This consolidates various emptiness checks across the codebase into a single +// reusable function. The function handles multiple value types with appropriate +// emptiness semantics for each. +// +// Returns true when encountering: +// - nil values (representing absence) +// - strings that are empty or contain only whitespace +// - numeric types equal to zero +// - boolean false +// - collections (slices, maps) with no elements +// +// Usage pattern: +// +// if isEmptyOrNil(configValue) { +// return NewValidationError("fieldName", "", "required field missing", "provide a value") +// } +func isEmptyOrNil(candidate any) bool { + // Handle nil case first + if candidate == nil { + return true + } + + // Type-specific emptiness checks using reflection-free approach + switch typedValue := candidate.(type) { + case string: + // String is empty if blank after trimming whitespace + return len(strings.TrimSpace(typedValue)) == 0 + case int: + return typedValue == 0 + case int8: + return typedValue == 0 + case int16: + return typedValue == 0 + case int32: + return typedValue == 0 + case int64: + return typedValue == 0 + case uint: + return typedValue == 0 + case uint8: + return typedValue == 0 + case uint16: + return typedValue == 0 + case uint32: + return typedValue == 0 + case uint64: + return typedValue == 0 + case float32: + return typedValue == 0.0 + case float64: + return typedValue == 0.0 + case bool: + // false represents empty boolean state + return typedValue == false + case []any: + return len(typedValue) == 0 + case map[string]any: + return len(typedValue) == 0 + } + + // Non-nil values of unrecognized types are considered non-empty + return false +} + +// getMapFieldAsString retrieves a string value from a configuration map with safe type handling. +// This function wraps the common pattern of extracting string fields from map[string]any structures +// that result from YAML parsing, providing consistent error behavior and logging. +// +// The function returns the fallback value in these scenarios: +// - Source map is nil +// - Requested key doesn't exist in map +// - Value at key is not a string type +// +// Parameters: +// - source: The configuration map to query +// - fieldKey: The key to look up in the map +// - fallback: Value returned when extraction fails +// +// Example usage: +// +// titleValue := getMapFieldAsString(frontmatter, "title", "") +// if titleValue == "" { +// return NewValidationError("title", "", "title required", "provide a title") +// } +func getMapFieldAsString(source map[string]any, fieldKey string, fallback string) string { + // Early return for nil map + if source == nil { + return fallback + } + + // Attempt to retrieve value + retrievedValue, keyFound := source[fieldKey] + if !keyFound { + return fallback + } + + // Verify type before returning + stringValue, isString := retrievedValue.(string) + if !isString { + validationHelpersLog.Printf("Type mismatch for key %q: expected string, found %T", fieldKey, retrievedValue) + return fallback + } + + return stringValue +} + +// getMapFieldAsMap retrieves a nested map value from a configuration map with safe type handling. +// This consolidates the pattern of extracting nested configuration sections while handling +// type mismatches gracefully. Returns nil when the field cannot be extracted as a map. +// +// Parameters: +// - source: The parent configuration map +// - fieldKey: The key identifying the nested map +// +// Example usage: +// +// toolsSection := getMapFieldAsMap(config, "tools") +// if toolsSection != nil { +// playwrightConfig := getMapFieldAsMap(toolsSection, "playwright") +// } +func getMapFieldAsMap(source map[string]any, fieldKey string) map[string]any { + // Guard against nil source + if source == nil { + return nil + } + + // Look up the field + retrievedValue, keyFound := source[fieldKey] + if !keyFound { + return nil + } + + // Type assert to nested map + mapValue, isMap := retrievedValue.(map[string]any) + if !isMap { + validationHelpersLog.Printf("Type mismatch for key %q: expected map[string]any, found %T", fieldKey, retrievedValue) + return nil + } + + return mapValue +} + +// getMapFieldAsBool retrieves a boolean value from a configuration map with safe type handling. +// This wraps the pattern of extracting boolean configuration flags while providing consistent +// fallback behavior when the value is missing or has an unexpected type. +// +// Parameters: +// - source: The configuration map to query +// - fieldKey: The key to look up +// - fallback: Value returned when extraction fails +// +// Example usage: +// +// sandboxEnabled := getMapFieldAsBool(config, "sandbox", false) +// if sandboxEnabled { +// // Enable sandbox mode +// } +func getMapFieldAsBool(source map[string]any, fieldKey string, fallback bool) bool { + // Handle nil source + if source == nil { + return fallback + } + + // Retrieve value from map + retrievedValue, keyFound := source[fieldKey] + if !keyFound { + return fallback + } + + // Verify boolean type + booleanValue, isBoolean := retrievedValue.(bool) + if !isBoolean { + validationHelpersLog.Printf("Type mismatch for key %q: expected bool, found %T", fieldKey, retrievedValue) + return fallback + } + + return booleanValue +} + +// getMapFieldAsInt retrieves an integer value from a configuration map with automatic numeric type conversion. +// This function handles the common pattern of extracting numeric config values that may be represented +// as various numeric types in YAML (int, int64, float64, uint64). It delegates to parseIntValue for +// the actual type conversion logic. +// +// Parameters: +// - source: The configuration map to query +// - fieldKey: The key to look up +// - fallback: Value returned when extraction or conversion fails +// +// Example usage: +// +// retentionDays := getMapFieldAsInt(config, "retention-days", 30) +// if err := validateIntRange(retentionDays, 1, 90, "retention-days"); err != nil { +// return err +// } +func getMapFieldAsInt(source map[string]any, fieldKey string, fallback int) int { + // Guard against nil source + if source == nil { + return fallback + } + + // Look up the value + retrievedValue, keyFound := source[fieldKey] + if !keyFound { + return fallback + } + + // Attempt numeric conversion using existing utility + convertedInt, conversionOk := parseIntValue(retrievedValue) + if !conversionOk { + validationHelpersLog.Printf("Failed to convert key %q to int: got %T", fieldKey, retrievedValue) + return fallback + } + + return convertedInt +} + +// dirExists verifies that a filesystem path exists and represents a directory. +// This consolidates directory existence checking patterns used throughout validation code. +// Returns false for empty paths, non-existent paths, or paths that reference files. +// +// Example usage: +// +// if !dirExists(workflowDirectory) { +// return NewValidationError("directory", workflowDirectory, "directory not found", "create the directory") +// } +func dirExists(targetPath string) bool { + // Reject empty paths immediately + if len(targetPath) == 0 { + validationHelpersLog.Print("Directory check received empty path") + return false + } + + // Stat the path to check existence and type + pathInfo, statError := os.Stat(targetPath) + if statError != nil { + validationHelpersLog.Printf("Directory check failed for %q: %v", targetPath, statError) + return false + } + + // Verify it's actually a directory + isDirectory := pathInfo.IsDir() + if !isDirectory { + validationHelpersLog.Printf("Path %q exists but is not a directory", targetPath) + } + + return isDirectory +} diff --git a/pkg/workflow/validation_helpers_test.go b/pkg/workflow/validation_helpers_test.go index a4cf492a6bf..600ceadd118 100644 --- a/pkg/workflow/validation_helpers_test.go +++ b/pkg/workflow/validation_helpers_test.go @@ -414,3 +414,334 @@ func TestValidateNonNegativeInt(t *testing.T) { assert.Contains(t, err.Error(), "must be a non-negative integer") }) } + +// TestIsEmptyOrNil tests the isEmptyOrNil helper function +func TestIsEmptyOrNil(t *testing.T) { + tests := []struct { + name string + value any + expected bool + }{ + // Nil values + {"nil value", nil, true}, + + // String values + {"empty string", "", true}, + {"whitespace string", " ", true}, + {"non-empty string", "hello", false}, + + // Integer values + {"zero int", 0, true}, + {"positive int", 5, false}, + {"negative int", -1, false}, + {"zero int64", int64(0), true}, + {"positive int64", int64(5), false}, + + // Unsigned integer values + {"zero uint", uint(0), true}, + {"positive uint", uint(5), false}, + {"zero uint64", uint64(0), true}, + {"positive uint64", uint64(5), false}, + + // Float values + {"zero float32", float32(0), true}, + {"positive float32", float32(5.5), false}, + {"zero float64", float64(0), true}, + {"positive float64", float64(5.5), false}, + + // Boolean values + {"false bool", false, true}, + {"true bool", true, false}, + + // Slice values + {"empty slice", []any{}, true}, + {"non-empty slice", []any{1, 2}, false}, + + // Map values + {"empty map", map[string]any{}, true}, + {"non-empty map", map[string]any{"key": "value"}, false}, + + // Other types + {"struct value", struct{ field string }{"value"}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isEmptyOrNil(tt.value) + assert.Equal(t, tt.expected, result, "isEmptyOrNil(%v) = %v, want %v", tt.value, result, tt.expected) + }) + } +} + +// TestGetMapFieldAsString tests the getMapFieldAsString helper function +func TestGetMapFieldAsString(t *testing.T) { + tests := []struct { + name string + m map[string]any + key string + defaultVal string + expected string + }{ + { + name: "extract existing string", + m: map[string]any{"title": "Test Title"}, + key: "title", + defaultVal: "", + expected: "Test Title", + }, + { + name: "missing key returns default", + m: map[string]any{"other": "value"}, + key: "title", + defaultVal: "default", + expected: "default", + }, + { + name: "non-string value returns default", + m: map[string]any{"title": 123}, + key: "title", + defaultVal: "default", + expected: "default", + }, + { + name: "nil map returns default", + m: nil, + key: "title", + defaultVal: "default", + expected: "default", + }, + { + name: "empty string value", + m: map[string]any{"title": ""}, + key: "title", + defaultVal: "default", + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := getMapFieldAsString(tt.m, tt.key, tt.defaultVal) + assert.Equal(t, tt.expected, result) + }) + } +} + +// TestGetMapFieldAsMap tests the getMapFieldAsMap helper function +func TestGetMapFieldAsMap(t *testing.T) { + tests := []struct { + name string + m map[string]any + key string + expected map[string]any + }{ + { + name: "extract existing nested map", + m: map[string]any{ + "network": map[string]any{ + "allowed-domains": "example.com", + }, + }, + key: "network", + expected: map[string]any{"allowed-domains": "example.com"}, + }, + { + name: "missing key returns nil", + m: map[string]any{"other": "value"}, + key: "network", + expected: nil, + }, + { + name: "non-map value returns nil", + m: map[string]any{"network": "not a map"}, + key: "network", + expected: nil, + }, + { + name: "nil map returns nil", + m: nil, + key: "network", + expected: nil, + }, + { + name: "empty nested map", + m: map[string]any{"network": map[string]any{}}, + key: "network", + expected: map[string]any{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := getMapFieldAsMap(tt.m, tt.key) + assert.Equal(t, tt.expected, result) + }) + } +} + +// TestGetMapFieldAsBool tests the getMapFieldAsBool helper function +func TestGetMapFieldAsBool(t *testing.T) { + tests := []struct { + name string + m map[string]any + key string + defaultVal bool + expected bool + }{ + { + name: "extract true value", + m: map[string]any{"enabled": true}, + key: "enabled", + defaultVal: false, + expected: true, + }, + { + name: "extract false value", + m: map[string]any{"enabled": false}, + key: "enabled", + defaultVal: true, + expected: false, + }, + { + name: "missing key returns default", + m: map[string]any{"other": true}, + key: "enabled", + defaultVal: true, + expected: true, + }, + { + name: "non-bool value returns default", + m: map[string]any{"enabled": "true"}, + key: "enabled", + defaultVal: false, + expected: false, + }, + { + name: "nil map returns default", + m: nil, + key: "enabled", + defaultVal: true, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := getMapFieldAsBool(tt.m, tt.key, tt.defaultVal) + assert.Equal(t, tt.expected, result) + }) + } +} + +// TestGetMapFieldAsInt tests the getMapFieldAsInt helper function +func TestGetMapFieldAsInt(t *testing.T) { + tests := []struct { + name string + m map[string]any + key string + defaultVal int + expected int + }{ + { + name: "extract int value", + m: map[string]any{"max-size": 100}, + key: "max-size", + defaultVal: 0, + expected: 100, + }, + { + name: "extract int64 value", + m: map[string]any{"max-size": int64(200)}, + key: "max-size", + defaultVal: 0, + expected: 200, + }, + { + name: "extract float64 value", + m: map[string]any{"max-size": float64(300)}, + key: "max-size", + defaultVal: 0, + expected: 300, + }, + { + name: "extract uint64 value", + m: map[string]any{"max-size": uint64(400)}, + key: "max-size", + defaultVal: 0, + expected: 400, + }, + { + name: "missing key returns default", + m: map[string]any{"other": 100}, + key: "max-size", + defaultVal: 50, + expected: 50, + }, + { + name: "non-numeric value returns default", + m: map[string]any{"max-size": "100"}, + key: "max-size", + defaultVal: 50, + expected: 50, + }, + { + name: "nil map returns default", + m: nil, + key: "max-size", + defaultVal: 50, + expected: 50, + }, + { + name: "zero value", + m: map[string]any{"max-size": 0}, + key: "max-size", + defaultVal: 100, + expected: 0, + }, + { + name: "negative value", + m: map[string]any{"max-size": -10}, + key: "max-size", + defaultVal: 100, + expected: -10, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := getMapFieldAsInt(tt.m, tt.key, tt.defaultVal) + assert.Equal(t, tt.expected, result) + }) + } +} + +// TestDirExists tests the dirExists helper function +func TestDirExists(t *testing.T) { + t.Run("empty path returns false", func(t *testing.T) { + result := dirExists("") + assert.False(t, result, "empty path should return false") + }) + + t.Run("non-existent path returns false", func(t *testing.T) { + result := dirExists("/nonexistent/path/to/directory") + assert.False(t, result, "non-existent path should return false") + }) + + t.Run("file path returns false", func(t *testing.T) { + // validation_helpers.go should exist and be a file, not a directory + result := dirExists("validation_helpers.go") + assert.False(t, result, "file path should return false") + }) + + t.Run("directory path returns true", func(t *testing.T) { + // Current directory should exist + result := dirExists(".") + assert.True(t, result, "current directory should return true") + }) + + t.Run("parent directory returns true", func(t *testing.T) { + // Parent directory should exist + result := dirExists("..") + assert.True(t, result, "parent directory should return true") + }) +} From b9135ed652be18a084a5695384ffbbf036efb450 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 7 Feb 2026 08:18:08 +0000 Subject: [PATCH 3/3] Fix linter error in isEmptyOrNil function Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/validation_helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index c1cb5e8a7bd..3c6a8e0d052 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -234,7 +234,7 @@ func isEmptyOrNil(candidate any) bool { return typedValue == 0.0 case bool: // false represents empty boolean state - return typedValue == false + return !typedValue case []any: return len(typedValue) == 0 case map[string]any: