diff --git a/pkg/workflow/engine_helpers.go b/pkg/workflow/engine_helpers.go index f2d9dd94d66..33e3adb4d4d 100644 --- a/pkg/workflow/engine_helpers.go +++ b/pkg/workflow/engine_helpers.go @@ -107,40 +107,6 @@ func GetBaseInstallationSteps(config EngineInstallConfig, workflowData *Workflow return steps } -// ExtractAgentIdentifier extracts the agent identifier (filename without extension) from an agent file path. -// This is used by the Copilot CLI which expects agent identifiers, not full paths. -// -// Parameters: -// - agentFile: The relative path to the agent file (e.g., ".github/agents/test-agent.md" or ".github/agents/test-agent.agent.md") -// -// Returns: -// - string: The agent identifier (e.g., "test-agent") -// -// Example: -// -// identifier := ExtractAgentIdentifier(".github/agents/my-agent.md") -// // Returns: "my-agent" -// -// identifier := ExtractAgentIdentifier(".github/agents/my-agent.agent.md") -// // Returns: "my-agent" -func ExtractAgentIdentifier(agentFile string) string { - engineHelpersLog.Printf("Extracting agent identifier from: %s", agentFile) - // Extract the base filename from the path - lastSlash := strings.LastIndex(agentFile, "/") - filename := agentFile - if lastSlash >= 0 { - filename = agentFile[lastSlash+1:] - } - - // Remove extensions in order: .agent.md, then .md, then .agent - // This handles all possible agent file naming conventions - filename = strings.TrimSuffix(filename, ".agent.md") - filename = strings.TrimSuffix(filename, ".md") - filename = strings.TrimSuffix(filename, ".agent") - - return filename -} - // ResolveAgentFilePath returns the properly quoted agent file path with GITHUB_WORKSPACE prefix. // This helper extracts the common pattern shared by Copilot, Codex, and Claude engines. // @@ -299,49 +265,6 @@ func FilterEnvForSecrets(env map[string]string, allowedNamesAndKeys []string) ma return filtered } -// GetHostedToolcachePathSetup returns a shell command that adds all runtime binaries -// from /opt/hostedtoolcache to PATH. This includes Node.js, Python, Go, Ruby, and other -// runtimes installed via actions/setup-* steps. -// -// The hostedtoolcache directory structure is: /opt/hostedtoolcache////bin -// This function generates a command that finds all bin directories and adds them to PATH. -// -// IMPORTANT: The command uses GH_AW_TOOL_BINS (computed by GetToolBinsSetup) which contains -// the specific tool paths from environment variables like GOROOT, JAVA_HOME, etc. These paths -// are computed on the RUNNER side and passed to the container as a literal value via --env, -// avoiding shell injection risks from variable expansion inside the container. -// -// This ensures that the version configured by actions/setup-* takes precedence over other -// versions that may exist in hostedtoolcache. Without this, the generic `find` command -// returns directories in alphabetical order, causing older versions (e.g., Go 1.22.12) -// to shadow newer ones (e.g., Go 1.25.6) because "1.22" < "1.25" alphabetically. -// -// This is used by all engine implementations (Copilot, Claude, Codex) to ensure consistent -// access to runtime tools inside the agent container. -// -// Returns: -// - string: A shell command that sets up PATH with all hostedtoolcache binaries -// -// Example output: -// -// export PATH="$GH_AW_TOOL_BINS$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')$PATH" -func GetHostedToolcachePathSetup() string { - // Use GH_AW_TOOL_BINS which is computed on the runner side by GetToolBinsSetup() - // and passed to the container via --env. This avoids shell injection risks from - // expanding variables like GOROOT inside the container. - // - // GH_AW_TOOL_BINS contains paths like "/opt/hostedtoolcache/go/1.25.6/x64/bin:" - // computed from GOROOT, JAVA_HOME, etc. on the runner where they are trusted. - - // Generic find for all other hostedtoolcache binaries (Node.js, Python, etc.) - genericFind := `$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')` - - // Build the raw PATH string, then sanitize it using GetSanitizedPATHExport() - // to remove empty elements, leading/trailing colons, and collapse multiple colons - rawPath := fmt.Sprintf(`$GH_AW_TOOL_BINS%s$PATH`, genericFind) - return GetSanitizedPATHExport(rawPath) -} - // GetNpmBinPathSetup returns a simple shell command that adds hostedtoolcache bin directories // to PATH. This is specifically for npm-installed CLIs (like Claude and Codex) that need // to find their binaries installed via `npm install -g`. @@ -364,85 +287,6 @@ func GetNpmBinPathSetup() string { return `export PATH="$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true` } -// GetSanitizedPATHExport returns a shell command that sets PATH to the given value -// with sanitization to remove security risks from malformed PATH entries. -// -// The sanitization removes: -// - Leading colons (e.g., ":/usr/bin" -> "/usr/bin") -// - Trailing colons (e.g., "/usr/bin:" -> "/usr/bin") -// - Empty elements (e.g., "/a::/b" -> "/a:/b", multiple colons collapsed to one) -// -// Empty PATH elements are a security risk because they cause the current directory -// to be searched for executables, which could allow malicious code execution. -// -// The sanitization logic is implemented in actions/setup/sh/sanitize_path.sh and -// is sourced at runtime from /opt/gh-aw/actions/sanitize_path.sh. -// -// Parameters: -// - rawPath: The unsanitized PATH value (may contain shell expansions like $PATH) -// -// Returns: -// - string: A shell command that sources the sanitize script to export the sanitized PATH -// -// Example: -// -// GetSanitizedPATHExport("$GH_AW_TOOL_BINS$PATH") -// // Returns: source /opt/gh-aw/actions/sanitize_path.sh "$GH_AW_TOOL_BINS$PATH" -func GetSanitizedPATHExport(rawPath string) string { - // Source the sanitize_path.sh script which handles: - // 1. Remove leading colons - // 2. Remove trailing colons - // 3. Collapse multiple colons into single colons - // 4. Export the sanitized PATH - return fmt.Sprintf(`source /opt/gh-aw/actions/sanitize_path.sh "%s"`, rawPath) -} - -// GetToolBinsSetup returns a shell command that computes the GH_AW_TOOL_BINS environment -// variable from specific tool paths (GOROOT, JAVA_HOME, etc.). -// -// This command should be run on the RUNNER side before invoking AWF, and the resulting -// GH_AW_TOOL_BINS should be passed to the container via --env. This ensures the paths -// are computed where they are trusted, avoiding shell injection risks. -// -// The computed paths are prepended to PATH (via GetHostedToolcachePathSetup) before the -// generic find results, ensuring versions set by actions/setup-* take precedence over -// alphabetically-earlier versions in hostedtoolcache. -// -// Returns: -// - string: A shell command that sets GH_AW_TOOL_BINS -// -// Example output when GOROOT=/opt/hostedtoolcache/go/1.25.6/x64 and JAVA_HOME=/opt/hostedtoolcache/Java/17.0.0/x64: -// -// GH_AW_TOOL_BINS="/opt/hostedtoolcache/go/1.25.6/x64/bin:/opt/hostedtoolcache/Java/17.0.0/x64/bin:" -func GetToolBinsSetup() string { - // Build GH_AW_TOOL_BINS from specific tool paths on the runner side. - // Each path is only added if the corresponding env var is set and non-empty. - // This runs on the runner where the env vars are trusted values from actions/setup-*. - // - // Tools with /bin subdirectory: - // - Go: Detected via `go env GOROOT` (actions/setup-go doesn't export GOROOT) - // - JAVA_HOME: Java installation root (actions/setup-java) - // - CARGO_HOME: Cargo/Rust installation (rustup) - // - GEM_HOME: Ruby gems (actions/setup-ruby) - // - CONDA: Conda installation - // - // Tools where the path IS the bin directory (no /bin suffix needed): - // - PIPX_BIN_DIR: pipx binary directory - // - SWIFT_PATH: Swift binary path - // - DOTNET_ROOT: .NET root (binaries are in root, not /bin) - return `GH_AW_TOOL_BINS=""; command -v go >/dev/null 2>&1 && GH_AW_TOOL_BINS="$(go env GOROOT)/bin:$GH_AW_TOOL_BINS"; [ -n "$JAVA_HOME" ] && GH_AW_TOOL_BINS="$JAVA_HOME/bin:$GH_AW_TOOL_BINS"; [ -n "$CARGO_HOME" ] && GH_AW_TOOL_BINS="$CARGO_HOME/bin:$GH_AW_TOOL_BINS"; [ -n "$GEM_HOME" ] && GH_AW_TOOL_BINS="$GEM_HOME/bin:$GH_AW_TOOL_BINS"; [ -n "$CONDA" ] && GH_AW_TOOL_BINS="$CONDA/bin:$GH_AW_TOOL_BINS"; [ -n "$PIPX_BIN_DIR" ] && GH_AW_TOOL_BINS="$PIPX_BIN_DIR:$GH_AW_TOOL_BINS"; [ -n "$SWIFT_PATH" ] && GH_AW_TOOL_BINS="$SWIFT_PATH:$GH_AW_TOOL_BINS"; [ -n "$DOTNET_ROOT" ] && GH_AW_TOOL_BINS="$DOTNET_ROOT:$GH_AW_TOOL_BINS"; export GH_AW_TOOL_BINS` -} - -// GetToolBinsEnvArg returns the AWF --env argument for passing GH_AW_TOOL_BINS to the container. -// This should be used after GetToolBinsSetup() has been run to compute the value. -// -// Returns: -// - []string: AWF arguments ["--env", "GH_AW_TOOL_BINS=$GH_AW_TOOL_BINS"] -func GetToolBinsEnvArg() []string { - // Pre-wrap in double quotes so shellEscapeArg preserves them (allowing shell expansion) - return []string{"--env", "\"GH_AW_TOOL_BINS=$GH_AW_TOOL_BINS\""} -} - // EngineHasValidateSecretStep checks if the engine provides a validate-secret step. // This is used to determine whether the secret_verification_result job output should be added. // diff --git a/pkg/workflow/engine_helpers_test.go b/pkg/workflow/engine_helpers_test.go index 54cdddb10cf..3f129360a93 100644 --- a/pkg/workflow/engine_helpers_test.go +++ b/pkg/workflow/engine_helpers_test.go @@ -214,85 +214,6 @@ func TestResolveAgentFilePathFormat(t *testing.T) { } } -// TestExtractAgentIdentifier tests extracting agent identifier from file paths -func TestExtractAgentIdentifier(t *testing.T) { - tests := []struct { - name string - input string - expected string - }{ - { - name: "basic agent file path", - input: ".github/agents/test-agent.md", - expected: "test-agent", - }, - { - name: "path with spaces", - input: ".github/agents/my agent file.md", - expected: "my agent file", - }, - { - name: "deeply nested path", - input: ".github/copilot/instructions/deep/nested/agent.md", - expected: "agent", - }, - { - name: "simple filename", - input: "agent.md", - expected: "agent", - }, - { - name: "path with special characters", - input: ".github/agents/test-agent_v2.0.md", - expected: "test-agent_v2.0", - }, - { - name: "cli-consistency-checker example", - input: ".github/agents/cli-consistency-checker.md", - expected: "cli-consistency-checker", - }, - { - name: "path without extension", - input: ".github/agents/test-agent", - expected: "test-agent", - }, - { - name: "custom agent file simple path", - input: ".github/agents/test-agent.agent.md", - expected: "test-agent", - }, - { - name: "custom agent file with path", - input: "../agents/technical-doc-writer.agent.md", - expected: "technical-doc-writer", - }, - { - name: "custom agent file with underscores", - input: ".github/agents/my_custom_agent.agent.md", - expected: "my_custom_agent", - }, - { - name: "agent file with only .agent extension", - input: ".github/agents/test-agent.agent", - expected: "test-agent", - }, - { - name: "agent file with .agent extension in path", - input: "../agents/my-agent.agent", - expected: "my-agent", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := ExtractAgentIdentifier(tt.input) - if result != tt.expected { - t.Errorf("ExtractAgentIdentifier(%q) = %q, want %q", tt.input, result, tt.expected) - } - }) - } -} - // TestShellVariableExpansionInAgentPath tests that agent paths allow shell variable expansion func TestShellVariableExpansionInAgentPath(t *testing.T) { agentFile := ".github/agents/test-agent.md" @@ -349,250 +270,6 @@ func TestShellEscapeArgWithFullyQuotedAgentPath(t *testing.T) { } } -// TestGetHostedToolcachePathSetup tests the hostedtoolcache PATH setup helper -func TestGetHostedToolcachePathSetup(t *testing.T) { - pathSetup := GetHostedToolcachePathSetup() - - // Should use find command to locate bin directories in hostedtoolcache - if !strings.Contains(pathSetup, "/opt/hostedtoolcache") { - t.Errorf("PATH setup should reference /opt/hostedtoolcache, got: %s", pathSetup) - } - - // Should look for bin directories - if !strings.Contains(pathSetup, "-name bin") { - t.Errorf("PATH setup should search for bin directories, got: %s", pathSetup) - } - - // Should use maxdepth 4 to reach /opt/hostedtoolcache////bin - if !strings.Contains(pathSetup, "-maxdepth 4") { - t.Errorf("PATH setup should use -maxdepth 4, got: %s", pathSetup) - } - - // Should suppress errors with 2>/dev/null - if !strings.Contains(pathSetup, "2>/dev/null") { - t.Errorf("PATH setup should suppress errors with 2>/dev/null, got: %s", pathSetup) - } - - // Should source the sanitize_path.sh script - if !strings.Contains(pathSetup, "source /opt/gh-aw/actions/sanitize_path.sh") { - t.Errorf("PATH setup should source sanitize_path.sh script, got: %s", pathSetup) - } - - // Should preserve existing PATH by including $PATH in the raw path - if !strings.Contains(pathSetup, "$PATH") { - t.Errorf("PATH setup should include $PATH to preserve existing PATH, got: %s", pathSetup) - } -} - -// TestGetHostedToolcachePathSetup_Consistency verifies the PATH setup produces consistent output -func TestGetHostedToolcachePathSetup_Consistency(t *testing.T) { - // Call multiple times to ensure consistent output - first := GetHostedToolcachePathSetup() - second := GetHostedToolcachePathSetup() - - if first != second { - t.Errorf("GetHostedToolcachePathSetup should return consistent results, got:\n%s\nvs:\n%s", first, second) - } -} - -// TestGetHostedToolcachePathSetup_UsesToolBins verifies that GetHostedToolcachePathSetup -// uses $GH_AW_TOOL_BINS to get specific tool paths computed by GetToolBinsSetup. -func TestGetHostedToolcachePathSetup_UsesToolBins(t *testing.T) { - pathSetup := GetHostedToolcachePathSetup() - - // Should use $GH_AW_TOOL_BINS for specific tool paths - if !strings.Contains(pathSetup, "$GH_AW_TOOL_BINS") { - t.Errorf("PATH setup should use $GH_AW_TOOL_BINS, got: %s", pathSetup) - } - - // Verify ordering: $GH_AW_TOOL_BINS should come BEFORE the find command - toolBinsIdx := strings.Index(pathSetup, "$GH_AW_TOOL_BINS") - findIdx := strings.Index(pathSetup, "find /opt/hostedtoolcache") - if toolBinsIdx > findIdx { - t.Errorf("$GH_AW_TOOL_BINS should come before find command in PATH setup, got: %s", pathSetup) - } -} - -// TestGetToolBinsSetup verifies that GetToolBinsSetup computes specific tool paths -// for all supported runtimes (Go, Java, Rust, Conda, Ruby, pipx, Swift, .NET). -func TestGetToolBinsSetup(t *testing.T) { - toolBinsSetup := GetToolBinsSetup() - - // Should use `go env GOROOT` for Go (actions/setup-go doesn't export GOROOT env var) - if !strings.Contains(toolBinsSetup, "command -v go") || !strings.Contains(toolBinsSetup, "$(go env GOROOT)/bin") { - t.Errorf("GetToolBinsSetup should use `go env GOROOT` for Go, got: %s", toolBinsSetup) - } - - // Should check JAVA_HOME for Java - if !strings.Contains(toolBinsSetup, "JAVA_HOME") || !strings.Contains(toolBinsSetup, "$JAVA_HOME/bin") { - t.Errorf("GetToolBinsSetup should handle JAVA_HOME, got: %s", toolBinsSetup) - } - - // Should check CARGO_HOME for Rust - if !strings.Contains(toolBinsSetup, "CARGO_HOME") || !strings.Contains(toolBinsSetup, "$CARGO_HOME/bin") { - t.Errorf("GetToolBinsSetup should handle CARGO_HOME, got: %s", toolBinsSetup) - } - - // Should check CONDA for Conda - if !strings.Contains(toolBinsSetup, `"$CONDA"`) || !strings.Contains(toolBinsSetup, "$CONDA/bin") { - t.Errorf("GetToolBinsSetup should handle CONDA, got: %s", toolBinsSetup) - } - - // Should check GEM_HOME for Ruby - if !strings.Contains(toolBinsSetup, "GEM_HOME") || !strings.Contains(toolBinsSetup, "$GEM_HOME/bin") { - t.Errorf("GetToolBinsSetup should handle GEM_HOME, got: %s", toolBinsSetup) - } - - // Should check PIPX_BIN_DIR for pipx (no /bin suffix) - if !strings.Contains(toolBinsSetup, "PIPX_BIN_DIR") || !strings.Contains(toolBinsSetup, "$PIPX_BIN_DIR:") { - t.Errorf("GetToolBinsSetup should handle PIPX_BIN_DIR, got: %s", toolBinsSetup) - } - - // Should check SWIFT_PATH for Swift (no /bin suffix) - if !strings.Contains(toolBinsSetup, "SWIFT_PATH") || !strings.Contains(toolBinsSetup, "$SWIFT_PATH:") { - t.Errorf("GetToolBinsSetup should handle SWIFT_PATH, got: %s", toolBinsSetup) - } - - // Should check DOTNET_ROOT for .NET (no /bin suffix) - if !strings.Contains(toolBinsSetup, "DOTNET_ROOT") || !strings.Contains(toolBinsSetup, "$DOTNET_ROOT:") { - t.Errorf("GetToolBinsSetup should handle DOTNET_ROOT, got: %s", toolBinsSetup) - } - - // Should export GH_AW_TOOL_BINS - if !strings.Contains(toolBinsSetup, "export GH_AW_TOOL_BINS") { - t.Errorf("GetToolBinsSetup should export GH_AW_TOOL_BINS, got: %s", toolBinsSetup) - } -} - -// TestGetToolBinsEnvArg verifies that GetToolBinsEnvArg returns the correct AWF argument. -func TestGetToolBinsEnvArg(t *testing.T) { - envArg := GetToolBinsEnvArg() - - if len(envArg) != 2 { - t.Errorf("GetToolBinsEnvArg should return 2 elements (--env and value), got: %d", len(envArg)) - } - - if envArg[0] != "--env" { - t.Errorf("First element should be --env, got: %s", envArg[0]) - } - - if envArg[1] != "\"GH_AW_TOOL_BINS=$GH_AW_TOOL_BINS\"" { - t.Errorf("Second element should be \"GH_AW_TOOL_BINS=$GH_AW_TOOL_BINS\" (with outer double quotes), got: %s", envArg[1]) - } -} - -// TestGetSanitizedPATHExport verifies that GetSanitizedPATHExport produces correct shell commands. -func TestGetSanitizedPATHExport(t *testing.T) { - result := GetSanitizedPATHExport("/usr/bin:/usr/local/bin") - - // Should source the sanitize_path.sh script from /opt/gh-aw/actions/ - if !strings.Contains(result, "source /opt/gh-aw/actions/sanitize_path.sh") { - t.Errorf("GetSanitizedPATHExport should source sanitize_path.sh, got: %s", result) - } - - // Should include the raw path as an argument - if !strings.Contains(result, "/usr/bin:/usr/local/bin") { - t.Errorf("GetSanitizedPATHExport should include the raw path, got: %s", result) - } -} - -// TestGetSanitizedPATHExport_ShellExecution tests that the sanitize_path.sh script -// correctly sanitizes various malformed PATH inputs when executed in a real shell. -// This test uses the script directly from actions/setup/sh/ since /opt/gh-aw/actions/ -// only exists at runtime. -func TestGetSanitizedPATHExport_ShellExecution(t *testing.T) { - // Get the path to the sanitize_path.sh script relative to this test file - _, thisFile, _, ok := runtime.Caller(0) - if !ok { - t.Fatal("Failed to get current file path") - } - // Navigate from pkg/workflow/ to actions/setup/sh/ - scriptPath := filepath.Join(filepath.Dir(thisFile), "..", "..", "actions", "setup", "sh", "sanitize_path.sh") - if _, err := os.Stat(scriptPath); os.IsNotExist(err) { - t.Fatalf("sanitize_path.sh script not found at %s", scriptPath) - } - - tests := []struct { - name string - input string - expected string - }{ - { - name: "already clean PATH", - input: "/usr/bin:/usr/local/bin", - expected: "/usr/bin:/usr/local/bin", - }, - { - name: "leading colon", - input: ":/usr/bin:/usr/local/bin", - expected: "/usr/bin:/usr/local/bin", - }, - { - name: "trailing colon", - input: "/usr/bin:/usr/local/bin:", - expected: "/usr/bin:/usr/local/bin", - }, - { - name: "multiple leading colons", - input: ":::/usr/bin:/usr/local/bin", - expected: "/usr/bin:/usr/local/bin", - }, - { - name: "multiple trailing colons", - input: "/usr/bin:/usr/local/bin:::", - expected: "/usr/bin:/usr/local/bin", - }, - { - name: "internal empty elements", - input: "/usr/bin::/usr/local/bin", - expected: "/usr/bin:/usr/local/bin", - }, - { - name: "multiple internal empty elements", - input: "/usr/bin:::/usr/local/bin", - expected: "/usr/bin:/usr/local/bin", - }, - { - name: "combined leading trailing and internal", - input: ":/usr/bin:::/usr/local/bin:", - expected: "/usr/bin:/usr/local/bin", - }, - { - name: "all colons", - input: ":::", - expected: "", - }, - { - name: "empty string", - input: "", - expected: "", - }, - { - name: "single path no colons", - input: "/usr/bin", - expected: "/usr/bin", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Source the script directly with the input and echo the resulting PATH - shellCmd := fmt.Sprintf(`source '%s' '%s' && echo "$PATH"`, scriptPath, tt.input) - - cmd := exec.Command("bash", "-c", shellCmd) - output, err := cmd.Output() - if err != nil { - t.Fatalf("Failed to execute shell command: %v\nCommand: %s", err, shellCmd) - } - - result := strings.TrimSpace(string(output)) - if result != tt.expected { - t.Errorf("Sanitized PATH = %q, want %q\nShell command: %s", result, tt.expected, shellCmd) - } - }) - } -} - func TestGetNpmBinPathSetup(t *testing.T) { pathSetup := GetNpmBinPathSetup() diff --git a/pkg/workflow/map_helpers.go b/pkg/workflow/map_helpers.go index a86572d92fb..a603058cfd2 100644 --- a/pkg/workflow/map_helpers.go +++ b/pkg/workflow/map_helpers.go @@ -18,14 +18,9 @@ // // Type Conversion: // - parseIntValue() - Safely parse numeric types to int with truncation warnings -// - isEmptyOrNil() - Check if a value is empty, nil, or zero // // Map Operations: // - filterMapKeys() - Create new map excluding specified keys -// - 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 // // These utilities handle common type conversion and map manipulation patterns that // occur frequently during YAML-to-struct parsing and configuration processing. @@ -33,8 +28,6 @@ package workflow import ( - "strings" - "github.com/github/gh-aw/pkg/logger" ) @@ -83,221 +76,3 @@ func filterMapKeys(original map[string]any, excludeKeys ...string) map[string]an } return result } - -// 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 - 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 { - mapHelpersLog.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 { - mapHelpersLog.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 { - mapHelpersLog.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 { - mapHelpersLog.Printf("Failed to convert key %q to int: got %T", fieldKey, retrievedValue) - return fallback - } - - return convertedInt -} diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index 86a84bb3e31..e2065c67a0a 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -7,12 +7,6 @@ // # 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 // - validateMountStringFormat() - Parses and validates a "source:dest:mode" mount string // // # Design Rationale @@ -31,8 +25,6 @@ package workflow import ( "errors" "fmt" - "slices" - "strconv" "strings" "github.com/github/gh-aw/pkg/logger" @@ -68,87 +60,6 @@ func validateIntRange(value, min, max int, fieldName string) error { return nil } -// ValidateRequired validates that a required field is not empty -func ValidateRequired(field, value string) error { - if strings.TrimSpace(value) == "" { - validationHelpersLog.Printf("Required field validation failed: field=%s", field) - return NewValidationError( - field, - value, - "field is required and cannot be empty", - fmt.Sprintf("Provide a non-empty value for '%s'", field), - ) - } - return nil -} - -// ValidateMaxLength validates that a field does not exceed maximum length -func ValidateMaxLength(field, value string, maxLength int) error { - if len(value) > maxLength { - return NewValidationError( - field, - value, - fmt.Sprintf("field exceeds maximum length of %d characters (actual: %d)", maxLength, len(value)), - fmt.Sprintf("Shorten '%s' to %d characters or less", field, maxLength), - ) - } - return nil -} - -// ValidateMinLength validates that a field meets minimum length requirement -func ValidateMinLength(field, value string, minLength int) error { - if len(value) < minLength { - return NewValidationError( - field, - value, - fmt.Sprintf("field is shorter than minimum length of %d characters (actual: %d)", minLength, len(value)), - fmt.Sprintf("Ensure '%s' is at least %d characters long", field, minLength), - ) - } - return nil -} - -// ValidateInList validates that a value is in an allowed list -func ValidateInList(field, value string, allowedValues []string) error { - if slices.Contains(allowedValues, value) { - return nil - } - - validationHelpersLog.Printf("List validation failed: field=%s, value=%s not in allowed list", field, value) - return NewValidationError( - field, - value, - fmt.Sprintf("value is not in allowed list: %v", allowedValues), - fmt.Sprintf("Choose one of the allowed values for '%s': %s", field, strings.Join(allowedValues, ", ")), - ) -} - -// ValidatePositiveInt validates that a value is a positive integer -func ValidatePositiveInt(field string, value int) error { - if value <= 0 { - return NewValidationError( - field, - strconv.Itoa(value), - "value must be a positive integer", - fmt.Sprintf("Provide a positive integer value for '%s'", field), - ) - } - return nil -} - -// ValidateNonNegativeInt validates that a value is a non-negative integer -func ValidateNonNegativeInt(field string, value int) error { - if value < 0 { - return NewValidationError( - field, - strconv.Itoa(value), - "value must be a non-negative integer", - fmt.Sprintf("Provide a non-negative integer value for '%s'", field), - ) - } - return nil -} - // validateMountStringFormat parses a mount string and validates its basic format. // Expected format: "source:destination:mode" where mode is "ro" or "rw". // Returns (source, dest, mode, nil) on success, or ("", "", "", error) on failure. diff --git a/pkg/workflow/validation_helpers_test.go b/pkg/workflow/validation_helpers_test.go index b6ea6df8cb9..c4885e23b2e 100644 --- a/pkg/workflow/validation_helpers_test.go +++ b/pkg/workflow/validation_helpers_test.go @@ -310,412 +310,6 @@ func TestValidateIntRangeWithRealWorldValues(t *testing.T) { } } -func TestValidateRequired(t *testing.T) { - t.Run("valid non-empty value", func(t *testing.T) { - err := ValidateRequired("title", "my title") - assert.NoError(t, err) - }) - - t.Run("empty value fails", func(t *testing.T) { - err := ValidateRequired("title", "") - require.Error(t, err) - assert.Contains(t, err.Error(), "field is required") - assert.Contains(t, err.Error(), "Provide a non-empty value") - }) - - t.Run("whitespace-only value fails", func(t *testing.T) { - err := ValidateRequired("title", " ") - require.Error(t, err) - assert.Contains(t, err.Error(), "cannot be empty") - }) -} - -func TestValidateMaxLength(t *testing.T) { - t.Run("value within limit", func(t *testing.T) { - err := ValidateMaxLength("title", "short", 100) - assert.NoError(t, err) - }) - - t.Run("value at limit", func(t *testing.T) { - err := ValidateMaxLength("title", "12345", 5) - assert.NoError(t, err) - }) - - t.Run("value exceeds limit", func(t *testing.T) { - err := ValidateMaxLength("title", "too long value", 5) - require.Error(t, err) - assert.Contains(t, err.Error(), "exceeds maximum length") - assert.Contains(t, err.Error(), "Shorten") - }) -} - -func TestValidateMinLength(t *testing.T) { - t.Run("value meets minimum", func(t *testing.T) { - err := ValidateMinLength("title", "hello", 3) - assert.NoError(t, err) - }) - - t.Run("value below minimum", func(t *testing.T) { - err := ValidateMinLength("title", "hi", 5) - require.Error(t, err) - assert.Contains(t, err.Error(), "shorter than minimum length") - assert.Contains(t, err.Error(), "at least 5 characters") - }) -} - -func TestValidateInList(t *testing.T) { - allowedValues := []string{"open", "closed", "draft"} - - t.Run("value in list", func(t *testing.T) { - err := ValidateInList("status", "open", allowedValues) - assert.NoError(t, err) - }) - - t.Run("value not in list", func(t *testing.T) { - err := ValidateInList("status", "invalid", allowedValues) - require.Error(t, err) - assert.Contains(t, err.Error(), "not in allowed list") - assert.Contains(t, err.Error(), "open, closed, draft") - }) -} - -func TestValidatePositiveInt(t *testing.T) { - t.Run("positive integer", func(t *testing.T) { - err := ValidatePositiveInt("count", 5) - assert.NoError(t, err) - }) - - t.Run("zero fails", func(t *testing.T) { - err := ValidatePositiveInt("count", 0) - require.Error(t, err) - assert.Contains(t, err.Error(), "must be a positive integer") - }) - - t.Run("negative fails", func(t *testing.T) { - err := ValidatePositiveInt("count", -1) - require.Error(t, err) - assert.Contains(t, err.Error(), "must be a positive integer") - }) -} - -func TestValidateNonNegativeInt(t *testing.T) { - t.Run("positive integer", func(t *testing.T) { - err := ValidateNonNegativeInt("count", 5) - assert.NoError(t, err) - }) - - t.Run("zero is valid", func(t *testing.T) { - err := ValidateNonNegativeInt("count", 0) - assert.NoError(t, err) - }) - - t.Run("negative fails", func(t *testing.T) { - err := ValidateNonNegativeInt("count", -1) - require.Error(t, err) - 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 fileutil.DirExists helper function func TestDirExists(t *testing.T) { t.Run("empty path returns false", func(t *testing.T) {