diff --git a/pkg/workflow/agentic_engine.go b/pkg/workflow/agentic_engine.go index 9a81f31dcb..4c2d67ff3b 100644 --- a/pkg/workflow/agentic_engine.go +++ b/pkg/workflow/agentic_engine.go @@ -15,8 +15,82 @@ var agenticEngineLog = logger.New("workflow:agentic_engine") // GitHubActionStep represents the YAML lines for a single step in a GitHub Actions workflow type GitHubActionStep []string -// CodingAgentEngine represents an AI coding agent that can be used as an engine to execute agentic workflows -type CodingAgentEngine interface { +// Interface Segregation Architecture +// +// The agentic engine interfaces follow the Interface Segregation Principle (ISP) to avoid +// forcing implementations to depend on methods they don't use. The architecture uses interface +// composition to provide flexibility while maintaining backward compatibility. +// +// Core Principles: +// 1. Focused interfaces with single responsibilities +// 2. Composition over monolithic interfaces +// 3. Backward compatibility via composite interface +// 4. Optional capabilities via interface type assertions +// +// Interface Hierarchy: +// +// Engine (core identity - required by all) +// ├── GetID() +// ├── GetDisplayName() +// ├── GetDescription() +// └── IsExperimental() +// +// CapabilityProvider (feature detection - optional) +// ├── SupportsToolsAllowlist() +// ├── SupportsHTTPTransport() +// ├── SupportsMaxTurns() +// ├── SupportsWebFetch() +// ├── SupportsWebSearch() +// └── SupportsFirewall() +// +// WorkflowExecutor (compilation - required) +// ├── GetDeclaredOutputFiles() +// ├── GetInstallationSteps() +// └── GetExecutionSteps() +// +// MCPConfigProvider (MCP servers - optional) +// └── RenderMCPConfig() +// +// LogParser (log analysis - optional) +// ├── ParseLogMetrics() +// ├── GetLogParserScriptId() +// └── GetLogFileForParsing() +// +// SecurityProvider (security features - optional) +// ├── GetDefaultDetectionModel() +// └── GetRequiredSecretNames() +// +// CodingAgentEngine (composite - backward compatibility) +// └── Composes all above interfaces +// +// Usage Patterns: +// +// 1. For code that only needs identity information: +// func processEngine(e Engine) { ... } +// +// 2. For code that needs capability checks: +// func checkCapabilities(cp CapabilityProvider) { ... } +// +// 3. For backward compatibility (existing code): +// func compile(engine CodingAgentEngine) { ... } +// +// Implementation: +// +// All engines embed BaseEngine which provides default implementations for all methods. +// Engines can override specific methods to provide custom behavior. +// +// Example: +// type MyEngine struct { +// BaseEngine +// } +// +// func (e *MyEngine) GetInstallationSteps(...) []GitHubActionStep { +// // Custom implementation +// } + +// Engine represents the core identity of an AI coding agent +// All engines must implement this interface to provide basic identification +type Engine interface { // GetID returns the unique identifier for this engine GetID() string @@ -28,7 +102,11 @@ type CodingAgentEngine interface { // IsExperimental returns true if this engine is experimental IsExperimental() bool +} +// CapabilityProvider detects what capabilities an engine supports +// Engines can optionally implement this to indicate feature support +type CapabilityProvider interface { // SupportsToolsAllowlist returns true if this engine supports MCP tool allow-listing SupportsToolsAllowlist() bool @@ -47,7 +125,11 @@ type CodingAgentEngine interface { // SupportsFirewall returns true if this engine supports network firewalling/sandboxing // When true, the engine can enforce network restrictions defined in the workflow SupportsFirewall() bool +} +// WorkflowExecutor handles workflow compilation and execution +// All engines must implement this to generate GitHub Actions steps +type WorkflowExecutor interface { // GetDeclaredOutputFiles returns a list of output files that this engine may produce // These files will be automatically uploaded as artifacts if they exist GetDeclaredOutputFiles() []string @@ -57,10 +139,18 @@ type CodingAgentEngine interface { // GetExecutionSteps returns the GitHub Actions steps for executing this engine GetExecutionSteps(workflowData *WorkflowData, logFile string) []GitHubActionStep +} +// MCPConfigProvider handles MCP (Model Context Protocol) configuration +// Engines that support MCP servers should implement this +type MCPConfigProvider interface { // RenderMCPConfig renders the MCP configuration for this engine to the given YAML builder RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) +} +// LogParser handles parsing and analyzing engine logs +// Engines can optionally implement this to provide detailed log parsing +type LogParser interface { // ParseLogMetrics extracts metrics from engine-specific log content ParseLogMetrics(logContent string, verbose bool) LogMetrics @@ -70,7 +160,11 @@ type CodingAgentEngine interface { // GetLogFileForParsing returns the log file path to use for JavaScript parsing in the workflow // This may be different from the stdout/stderr log file if the engine produces separate detailed logs GetLogFileForParsing() string +} +// SecurityProvider handles security-related configuration +// Engines can optionally implement this to provide security features +type SecurityProvider interface { // GetDefaultDetectionModel returns the default model to use for threat detection // If empty, no default model is applied and the engine uses its standard default GetDefaultDetectionModel() string @@ -81,6 +175,18 @@ type CodingAgentEngine interface { GetRequiredSecretNames(workflowData *WorkflowData) []string } +// CodingAgentEngine is a composite interface that combines all focused interfaces +// This maintains backward compatibility with existing code while allowing more flexibility +// Implementations can choose to implement only the interfaces they need by embedding BaseEngine +type CodingAgentEngine interface { + Engine + CapabilityProvider + WorkflowExecutor + MCPConfigProvider + LogParser + SecurityProvider +} + // BaseEngine provides common functionality for agentic engines type BaseEngine struct { id string diff --git a/pkg/workflow/agentic_engine_interfaces_test.go b/pkg/workflow/agentic_engine_interfaces_test.go new file mode 100644 index 0000000000..48e2456d68 --- /dev/null +++ b/pkg/workflow/agentic_engine_interfaces_test.go @@ -0,0 +1,365 @@ +package workflow + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestInterfaceSegregation validates that the interface segregation is properly implemented +func TestInterfaceSegregation(t *testing.T) { + t.Run("all engines implement CodingAgentEngine composite interface", func(t *testing.T) { + registry := NewEngineRegistry() + engines := registry.GetAllEngines() + + for _, engine := range engines { + // Verify each engine implements the composite interface + // All engines are returned as CodingAgentEngine from registry + assert.NotNil(t, engine, "Engine should not be nil") + } + }) + + t.Run("all engines implement Engine core interface", func(t *testing.T) { + registry := NewEngineRegistry() + engines := registry.GetAllEngines() + + for _, engine := range engines { + // Verify core Engine interface + _, ok := engine.(Engine) + assert.True(t, ok, "Engine %s should implement Engine interface", engine.GetID()) + + // Test required methods + assert.NotEmpty(t, engine.GetID(), "GetID should return non-empty string") + assert.NotEmpty(t, engine.GetDisplayName(), "GetDisplayName should return non-empty string") + assert.NotEmpty(t, engine.GetDescription(), "GetDescription should return non-empty string") + // IsExperimental can be true or false, just verify it exists + _ = engine.IsExperimental() + } + }) + + t.Run("all engines implement CapabilityProvider interface", func(t *testing.T) { + registry := NewEngineRegistry() + engines := registry.GetAllEngines() + + for _, engine := range engines { + // Verify CapabilityProvider interface + _, ok := engine.(CapabilityProvider) + assert.True(t, ok, "Engine %s should implement CapabilityProvider", engine.GetID()) + + // Test capability methods (values can be true/false, just verify they exist) + _ = engine.SupportsToolsAllowlist() + _ = engine.SupportsHTTPTransport() + _ = engine.SupportsMaxTurns() + _ = engine.SupportsWebFetch() + _ = engine.SupportsWebSearch() + _ = engine.SupportsFirewall() + } + }) + + t.Run("all engines implement WorkflowExecutor interface", func(t *testing.T) { + registry := NewEngineRegistry() + engines := registry.GetAllEngines() + + for _, engine := range engines { + // Verify WorkflowExecutor interface + _, ok := engine.(WorkflowExecutor) + assert.True(t, ok, "Engine %s should implement WorkflowExecutor", engine.GetID()) + + // Create minimal workflow data for testing + workflowData := &WorkflowData{ + Name: "test-workflow", + ParsedTools: &ToolsConfig{}, + Tools: map[string]any{}, + } + + // Test GetDeclaredOutputFiles (can return empty list) + outputFiles := engine.GetDeclaredOutputFiles() + assert.NotNil(t, outputFiles, "GetDeclaredOutputFiles should not return nil") + + // Test GetInstallationSteps (can return empty list) + installSteps := engine.GetInstallationSteps(workflowData) + assert.NotNil(t, installSteps, "GetInstallationSteps should not return nil") + + // Test GetExecutionSteps (can return empty list) + execSteps := engine.GetExecutionSteps(workflowData, "/tmp/test.log") + assert.NotNil(t, execSteps, "GetExecutionSteps should not return nil") + } + }) + + t.Run("all engines implement MCPConfigProvider interface", func(t *testing.T) { + registry := NewEngineRegistry() + engines := registry.GetAllEngines() + + for _, engine := range engines { + // Verify MCPConfigProvider interface + _, ok := engine.(MCPConfigProvider) + assert.True(t, ok, "Engine %s should implement MCPConfigProvider", engine.GetID()) + + // Test RenderMCPConfig (method exists, may write nothing or something) + var yaml strings.Builder + workflowData := &WorkflowData{ + Name: "test-workflow", + ParsedTools: &ToolsConfig{}, + Tools: map[string]any{}, + } + // Should not panic + engine.RenderMCPConfig(&yaml, map[string]any{}, []string{}, workflowData) + } + }) + + t.Run("all engines implement LogParser interface", func(t *testing.T) { + registry := NewEngineRegistry() + engines := registry.GetAllEngines() + + for _, engine := range engines { + // Verify LogParser interface + _, ok := engine.(LogParser) + assert.True(t, ok, "Engine %s should implement LogParser", engine.GetID()) + + // Test GetLogParserScriptId (can return empty string) + scriptId := engine.GetLogParserScriptId() + _ = scriptId // Can be empty or non-empty + + // Test GetLogFileForParsing (should return non-empty path) + logFile := engine.GetLogFileForParsing() + assert.NotEmpty(t, logFile, "GetLogFileForParsing should return non-empty path") + + // Test ParseLogMetrics (can return empty metrics) + metrics := engine.ParseLogMetrics("test log content", false) + assert.NotNil(t, metrics, "ParseLogMetrics should not return nil") + } + }) + + t.Run("all engines implement SecurityProvider interface", func(t *testing.T) { + registry := NewEngineRegistry() + engines := registry.GetAllEngines() + + for _, engine := range engines { + // Verify SecurityProvider interface + _, ok := engine.(SecurityProvider) + assert.True(t, ok, "Engine %s should implement SecurityProvider", engine.GetID()) + + // Test GetDefaultDetectionModel (can return empty string) + model := engine.GetDefaultDetectionModel() + _ = model // Can be empty or non-empty + + // Test GetRequiredSecretNames (can return empty list) + workflowData := &WorkflowData{ + Name: "test-workflow", + ParsedTools: &ToolsConfig{}, + Tools: map[string]any{}, + } + secrets := engine.GetRequiredSecretNames(workflowData) + assert.NotNil(t, secrets, "GetRequiredSecretNames should not return nil for engine %s", engine.GetID()) + } + }) +} + +// TestInterfaceComposition validates that interface composition works correctly +func TestInterfaceComposition(t *testing.T) { + t.Run("CodingAgentEngine composes all sub-interfaces", func(t *testing.T) { + // Get an engine instance + registry := NewEngineRegistry() + engine, err := registry.GetEngine("copilot") + require.NoError(t, err) + + // Verify it can also be cast to individual interfaces + _, ok := engine.(Engine) + assert.True(t, ok, "Should be able to cast CodingAgentEngine to Engine") + + _, ok = engine.(CapabilityProvider) + assert.True(t, ok, "Should be able to cast CodingAgentEngine to CapabilityProvider") + + _, ok = engine.(WorkflowExecutor) + assert.True(t, ok, "Should be able to cast CodingAgentEngine to WorkflowExecutor") + + _, ok = engine.(MCPConfigProvider) + assert.True(t, ok, "Should be able to cast CodingAgentEngine to MCPConfigProvider") + + _, ok = engine.(LogParser) + assert.True(t, ok, "Should be able to cast CodingAgentEngine to LogParser") + + _, ok = engine.(SecurityProvider) + assert.True(t, ok, "Should be able to cast CodingAgentEngine to SecurityProvider") + }) +} + +// TestSpecificInterfaceUsage demonstrates using specific interfaces +func TestSpecificInterfaceUsage(t *testing.T) { + t.Run("using only Engine interface", func(t *testing.T) { + registry := NewEngineRegistry() + + // Function that only needs Engine interface + checkEngineIdentity := func(e Engine) bool { + return e.GetID() != "" && e.GetDisplayName() != "" + } + + // All engines should satisfy this + for _, engine := range registry.GetAllEngines() { + assert.True(t, checkEngineIdentity(engine), "Engine %s should have valid identity", engine.GetID()) + } + }) + + t.Run("using only CapabilityProvider interface", func(t *testing.T) { + registry := NewEngineRegistry() + + // Function that only needs CapabilityProvider interface + checkCapabilities := func(cp CapabilityProvider) map[string]bool { + return map[string]bool{ + "tools_allowlist": cp.SupportsToolsAllowlist(), + "http_transport": cp.SupportsHTTPTransport(), + "max_turns": cp.SupportsMaxTurns(), + "web_fetch": cp.SupportsWebFetch(), + "web_search": cp.SupportsWebSearch(), + "firewall": cp.SupportsFirewall(), + } + } + + // All engines should satisfy this + for _, engine := range registry.GetAllEngines() { + caps := checkCapabilities(engine) + assert.NotNil(t, caps, "Engine %s should have capabilities", engine.GetID()) + assert.Len(t, caps, 6, "Should have 6 capability flags") + } + }) + + t.Run("using only WorkflowExecutor interface", func(t *testing.T) { + registry := NewEngineRegistry() + + // Function that only needs WorkflowExecutor interface + canExecuteWorkflow := func(we WorkflowExecutor) bool { + workflowData := &WorkflowData{ + Name: "test", + ParsedTools: &ToolsConfig{}, + Tools: map[string]any{}, + } + installSteps := we.GetInstallationSteps(workflowData) + execSteps := we.GetExecutionSteps(workflowData, "/tmp/test.log") + return installSteps != nil && execSteps != nil + } + + // All engines should satisfy this + for _, engine := range registry.GetAllEngines() { + assert.True(t, canExecuteWorkflow(engine), "Engine %s should be able to execute workflows", engine.GetID()) + } + }) +} + +// TestBaseEngineImplementsAllInterfaces verifies BaseEngine provides default implementations +func TestBaseEngineImplementsAllInterfaces(t *testing.T) { + // Create a BaseEngine instance + base := &BaseEngine{ + id: "test", + displayName: "Test Engine", + description: "A test engine", + experimental: false, + supportsToolsAllowlist: true, + supportsHTTPTransport: true, + supportsMaxTurns: true, + supportsWebFetch: true, + supportsWebSearch: true, + supportsFirewall: true, + } + + // Verify Engine interface methods + assert.Equal(t, "test", base.GetID()) + assert.Equal(t, "Test Engine", base.GetDisplayName()) + assert.Equal(t, "A test engine", base.GetDescription()) + assert.False(t, base.IsExperimental()) + + // Verify CapabilityProvider interface methods + assert.True(t, base.SupportsToolsAllowlist()) + assert.True(t, base.SupportsHTTPTransport()) + assert.True(t, base.SupportsMaxTurns()) + assert.True(t, base.SupportsWebFetch()) + assert.True(t, base.SupportsWebSearch()) + assert.True(t, base.SupportsFirewall()) + + // Verify default implementations + assert.Empty(t, base.GetDeclaredOutputFiles()) + assert.Empty(t, base.GetDefaultDetectionModel()) + assert.Equal(t, "/tmp/gh-aw/agent-stdio.log", base.GetLogFileForParsing()) + + workflowData := &WorkflowData{ + Name: "test", + ParsedTools: &ToolsConfig{}, + Tools: map[string]any{}, + } + assert.Empty(t, base.GetRequiredSecretNames(workflowData)) +} + +// TestEngineCapabilityVariety validates that different engines have different capabilities +func TestEngineCapabilityVariety(t *testing.T) { + registry := NewEngineRegistry() + + copilot, _ := registry.GetEngine("copilot") + claude, _ := registry.GetEngine("claude") + codex, _ := registry.GetEngine("codex") + custom, _ := registry.GetEngine("custom") + + // Test that capabilities differ across engines + t.Run("copilot capabilities", func(t *testing.T) { + assert.True(t, copilot.SupportsToolsAllowlist()) + assert.True(t, copilot.SupportsHTTPTransport()) + assert.False(t, copilot.SupportsMaxTurns()) + assert.True(t, copilot.SupportsWebFetch()) + assert.False(t, copilot.SupportsWebSearch()) + assert.True(t, copilot.SupportsFirewall()) + assert.False(t, copilot.IsExperimental()) + }) + + t.Run("claude capabilities", func(t *testing.T) { + assert.True(t, claude.SupportsToolsAllowlist()) + assert.True(t, claude.SupportsHTTPTransport()) + assert.True(t, claude.SupportsMaxTurns()) + assert.True(t, claude.SupportsWebFetch()) + assert.True(t, claude.SupportsWebSearch()) + assert.True(t, claude.SupportsFirewall()) + assert.True(t, claude.IsExperimental()) + }) + + t.Run("codex capabilities", func(t *testing.T) { + assert.True(t, codex.SupportsToolsAllowlist()) + assert.True(t, codex.SupportsHTTPTransport()) + assert.False(t, codex.SupportsMaxTurns()) + assert.False(t, codex.SupportsWebFetch()) + assert.True(t, codex.SupportsWebSearch()) + assert.True(t, codex.SupportsFirewall()) + assert.True(t, codex.IsExperimental()) + }) + + t.Run("custom capabilities", func(t *testing.T) { + assert.False(t, custom.SupportsToolsAllowlist()) + assert.False(t, custom.SupportsHTTPTransport()) + assert.True(t, custom.SupportsMaxTurns()) + assert.False(t, custom.SupportsWebFetch()) + assert.False(t, custom.SupportsWebSearch()) + assert.False(t, custom.IsExperimental()) + }) +} + +// TestEngineRegistryAcceptsEngineInterface validates that EngineRegistry works with the Engine interface +func TestEngineRegistryAcceptsEngineInterface(t *testing.T) { + registry := NewEngineRegistry() + + // Create a minimal engine that only implements Engine interface (via BaseEngine) + minimalEngine := &ClaudeEngine{ + BaseEngine: BaseEngine{ + id: "minimal-test", + displayName: "Minimal Test Engine", + description: "Minimal engine for testing", + experimental: false, + }, + } + + // Should be able to register it + registry.Register(minimalEngine) + + // Should be able to retrieve it + retrieved, err := registry.GetEngine("minimal-test") + require.NoError(t, err) + assert.Equal(t, "minimal-test", retrieved.GetID()) + assert.Equal(t, "Minimal Test Engine", retrieved.GetDisplayName()) +} diff --git a/pkg/workflow/custom_engine.go b/pkg/workflow/custom_engine.go index f2c8b56d65..f371e4344d 100644 --- a/pkg/workflow/custom_engine.go +++ b/pkg/workflow/custom_engine.go @@ -36,7 +36,7 @@ func (e *CustomEngine) GetRequiredSecretNames(workflowData *WorkflowData) []stri // Custom engine doesn't have predefined secrets // User-defined steps should explicitly reference secrets they need // MCP gateway API key is added if MCP servers are present - var secrets []string + secrets := []string{} if HasMCPServers(workflowData) { secrets = append(secrets, "MCP_GATEWAY_API_KEY")