diff --git a/pkg/workflow/engine.go b/pkg/workflow/engine.go index 49b89a57b6..bbfb6022fa 100644 --- a/pkg/workflow/engine.go +++ b/pkg/workflow/engine.go @@ -257,31 +257,6 @@ func (c *Compiler) ExtractEngineConfig(frontmatter map[string]any) (string, *Eng return "", nil } -// validateEngine validates that the given engine ID is supported -func (c *Compiler) validateEngine(engineID string) error { - if engineID == "" { - engineLog.Print("No engine ID specified, will use default") - return nil // Empty engine is valid (will use default) - } - - engineLog.Printf("Validating engine ID: %s", engineID) - - // First try exact match - if c.engineRegistry.IsValidEngine(engineID) { - engineLog.Printf("Engine ID %s is valid (exact match)", engineID) - return nil - } - - // Try prefix match for backward compatibility (e.g., "codex-experimental") - engine, err := c.engineRegistry.GetEngineByPrefix(engineID) - if err == nil { - engineLog.Printf("Engine ID %s matched by prefix to: %s", engineID, engine.GetID()) - } else { - engineLog.Printf("Engine ID %s not found: %v", engineID, err) - } - return err -} - // getAgenticEngine returns the agentic engine for the given engine setting func (c *Compiler) getAgenticEngine(engineSetting string) (CodingAgentEngine, error) { if engineSetting == "" { @@ -311,57 +286,6 @@ func (c *Compiler) getAgenticEngine(engineSetting string) (CodingAgentEngine, er return engine, err } -// validateSingleEngineSpecification validates that only one engine field exists across all files -func (c *Compiler) validateSingleEngineSpecification(mainEngineSetting string, includedEnginesJSON []string) (string, error) { - var allEngines []string - - // Add main engine if specified - if mainEngineSetting != "" { - allEngines = append(allEngines, mainEngineSetting) - } - - // Add included engines - for _, engineJSON := range includedEnginesJSON { - if engineJSON != "" { - allEngines = append(allEngines, engineJSON) - } - } - - // Check count - if len(allEngines) == 0 { - return "", nil // No engine specified anywhere, will use default - } - - if len(allEngines) > 1 { - return "", fmt.Errorf("multiple engine fields found. Only one engine field is allowed across the main workflow and all included files. Remove engine specifications to have only one") - } - - // Exactly one engine found - parse and return it - if mainEngineSetting != "" { - return mainEngineSetting, nil - } - - // Must be from included file - var firstEngine any - if err := json.Unmarshal([]byte(includedEnginesJSON[0]), &firstEngine); err != nil { - return "", fmt.Errorf("failed to parse included engine configuration: %w", err) - } - - // Handle string format - if engineStr, ok := firstEngine.(string); ok { - return engineStr, nil - } else if engineObj, ok := firstEngine.(map[string]any); ok { - // Handle object format - return the ID - if id, hasID := engineObj["id"]; hasID { - if idStr, ok := id.(string); ok { - return idStr, nil - } - } - } - - return "", fmt.Errorf("invalid engine configuration in included file") -} - // extractEngineConfigFromJSON parses engine configuration from JSON string (from included files) func (c *Compiler) extractEngineConfigFromJSON(engineJSON string) (*EngineConfig, error) { if engineJSON == "" { diff --git a/pkg/workflow/engine_validation.go b/pkg/workflow/engine_validation.go new file mode 100644 index 0000000000..1e74dff88d --- /dev/null +++ b/pkg/workflow/engine_validation.go @@ -0,0 +1,118 @@ +// Package workflow provides engine validation for agentic workflows. +// +// # Engine Validation +// +// This file validates engine configurations used in agentic workflows. +// Validation ensures that engine IDs are supported and that only one engine +// specification exists across the main workflow and all included files. +// +// # Validation Functions +// +// - validateEngine() - Validates that a given engine ID is supported +// - validateSingleEngineSpecification() - Validates that only one engine field exists across all files +// +// # Validation Pattern: Engine Registry +// +// Engine validation uses the compiler's engine registry: +// - Supports exact engine ID matching (e.g., "copilot", "claude") +// - Supports prefix matching for backward compatibility (e.g., "codex-experimental") +// - Empty engine IDs are valid and use the default engine +// - Detailed logging of validation steps for debugging +// +// # When to Add Validation Here +// +// Add validation to this file when: +// - It validates engine IDs or engine configurations +// - It checks engine registry entries +// - It validates engine-specific settings +// - It validates engine field consistency across imports +// +// For engine configuration extraction, see engine.go. +// For general validation, see validation.go. +// For detailed documentation, see specs/validation-architecture.md +package workflow + +import ( + "encoding/json" + "fmt" + + "github.com/githubnext/gh-aw/pkg/logger" +) + +var engineValidationLog = logger.New("workflow:engine_validation") + +// validateEngine validates that the given engine ID is supported +func (c *Compiler) validateEngine(engineID string) error { + if engineID == "" { + engineValidationLog.Print("No engine ID specified, will use default") + return nil // Empty engine is valid (will use default) + } + + engineValidationLog.Printf("Validating engine ID: %s", engineID) + + // First try exact match + if c.engineRegistry.IsValidEngine(engineID) { + engineValidationLog.Printf("Engine ID %s is valid (exact match)", engineID) + return nil + } + + // Try prefix match for backward compatibility (e.g., "codex-experimental") + engine, err := c.engineRegistry.GetEngineByPrefix(engineID) + if err == nil { + engineValidationLog.Printf("Engine ID %s matched by prefix to: %s", engineID, engine.GetID()) + } else { + engineValidationLog.Printf("Engine ID %s not found: %v", engineID, err) + } + return err +} + +// validateSingleEngineSpecification validates that only one engine field exists across all files +func (c *Compiler) validateSingleEngineSpecification(mainEngineSetting string, includedEnginesJSON []string) (string, error) { + var allEngines []string + + // Add main engine if specified + if mainEngineSetting != "" { + allEngines = append(allEngines, mainEngineSetting) + } + + // Add included engines + for _, engineJSON := range includedEnginesJSON { + if engineJSON != "" { + allEngines = append(allEngines, engineJSON) + } + } + + // Check count + if len(allEngines) == 0 { + return "", nil // No engine specified anywhere, will use default + } + + if len(allEngines) > 1 { + return "", fmt.Errorf("multiple engine fields found. Only one engine field is allowed across the main workflow and all included files. Remove engine specifications to have only one") + } + + // Exactly one engine found - parse and return it + if mainEngineSetting != "" { + return mainEngineSetting, nil + } + + // Must be from included file + var firstEngine any + if err := json.Unmarshal([]byte(includedEnginesJSON[0]), &firstEngine); err != nil { + return "", fmt.Errorf("failed to parse included engine configuration: %w", err) + } + + // Handle string format + if engineStr, ok := firstEngine.(string); ok { + return engineStr, nil + } else if engineObj, ok := firstEngine.(map[string]any); ok { + // Handle object format - return the ID + if id, hasID := engineObj["id"]; hasID { + if idStr, ok := id.(string); ok { + return idStr, nil + } + } + } + + return "", fmt.Errorf("invalid engine configuration in included file") +}