From 2a3f543ae146a46a3da383fc545e769002c8b99f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 02:59:16 +0000 Subject: [PATCH 1/2] Initial plan From 42bcf58617e4bd82b52b4299663bcf8f4d4ad87f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 03:20:34 +0000 Subject: [PATCH 2/2] feat: introduce EngineDefinition, EngineCatalog, and ResolvedEngineTarget (Phase 1) - Create engine_definition.go with EngineDefinition, EngineCatalog, ResolvedEngineTarget, ProviderSelection, ModelSelection, AuthBinding types - NewEngineCatalog registers claude/codex/copilot/gemini as built-in entries - EngineCatalog.Resolve() handles exact lookup, prefix fallback, and formatted validation errors matching the existing validateEngine style - Add engineCatalog field to Compiler, initialized in NewCompiler() - Update compiler_orchestrator_engine.go to use catalog.Resolve() instead of separate validateEngine + getAgenticEngine calls - Add engine_definition_test.go with 6 regression tests covering all built-ins, legacy string/object formats, prefix fallback, unknown engine errors (including docs URL validation), config passthrough, custom registration Closes #20416 Phase 1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_orchestrator_engine.go | 18 +- pkg/workflow/compiler_types.go | 2 + pkg/workflow/engine_definition.go | 185 +++++++++++++++++++ pkg/workflow/engine_definition_test.go | 130 +++++++++++++ 4 files changed, 325 insertions(+), 10 deletions(-) create mode 100644 pkg/workflow/engine_definition.go create mode 100644 pkg/workflow/engine_definition_test.go diff --git a/pkg/workflow/compiler_orchestrator_engine.go b/pkg/workflow/compiler_orchestrator_engine.go index 5ceaf329933..d0bb4287462 100644 --- a/pkg/workflow/compiler_orchestrator_engine.go +++ b/pkg/workflow/compiler_orchestrator_engine.go @@ -199,19 +199,17 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean } } - // Validate the engine setting - orchestratorEngineLog.Printf("Validating engine setting: %s", engineSetting) - if err := c.validateEngine(engineSetting); err != nil { - orchestratorEngineLog.Printf("Engine validation failed: %v", err) - return nil, err - } - - // Get the agentic engine instance - agenticEngine, err := c.getAgenticEngine(engineSetting) + // Validate the engine setting and resolve the runtime adapter via the catalog. + // This performs exact catalog lookup, prefix fallback, and returns a formatted + // validation error for unknown engines — replacing the separate validateEngine + // and getAgenticEngine calls. + orchestratorEngineLog.Printf("Resolving engine setting: %s", engineSetting) + resolvedEngine, err := c.engineCatalog.Resolve(engineSetting, engineConfig) if err != nil { - orchestratorEngineLog.Printf("Failed to get agentic engine: %v", err) + orchestratorEngineLog.Printf("Engine resolution failed: %v", err) return nil, err } + agenticEngine := resolvedEngine.Runtime log.Printf("AI engine: %s (%s)", agenticEngine.GetDisplayName(), engineSetting) if agenticEngine.IsExperimental() && c.verbose { diff --git a/pkg/workflow/compiler_types.go b/pkg/workflow/compiler_types.go index d56dee845ca..2043ee51dd2 100644 --- a/pkg/workflow/compiler_types.go +++ b/pkg/workflow/compiler_types.go @@ -98,6 +98,7 @@ type Compiler struct { actionTag string // Override action SHA or tag for actions/setup (when set, overrides actionMode to release) jobManager *JobManager // Manages jobs and dependencies engineRegistry *EngineRegistry // Registry of available agentic engines + engineCatalog *EngineCatalog // Catalog of engine definitions backed by the registry fileTracker FileTracker // Optional file tracker for tracking created files warningCount int // Number of warnings encountered during compilation stepOrderTracker *StepOrderTracker // Tracks step ordering for validation @@ -136,6 +137,7 @@ func NewCompiler(opts ...CompilerOption) *Compiler { actionMode: DetectActionMode(version), // Auto-detect action mode based on version jobManager: NewJobManager(), engineRegistry: GetGlobalEngineRegistry(), + engineCatalog: NewEngineCatalog(GetGlobalEngineRegistry()), stepOrderTracker: NewStepOrderTracker(), artifactManager: NewArtifactManager(), actionPinWarnings: make(map[string]bool), // Initialize warning cache diff --git a/pkg/workflow/engine_definition.go b/pkg/workflow/engine_definition.go new file mode 100644 index 00000000000..e3154ad2cb1 --- /dev/null +++ b/pkg/workflow/engine_definition.go @@ -0,0 +1,185 @@ +// This file defines the engine definition layer: declarative metadata types for AI engines, +// a catalog of registered definitions, and a resolved target that combines definition, +// config, and runtime adapter. +// +// # Architecture +// +// The engine definition layer sits on top of the existing EngineRegistry runtime layer: +// +// EngineDefinition – declarative metadata for a single engine entry +// EngineCatalog – registry of EngineDefinition entries with a Resolve() method +// ResolvedEngineTarget – result of resolving an engine ID: definition + config + runtime +// +// The existing EngineRegistry and CodingAgentEngine interfaces are unchanged; the catalog +// is an additional layer that maps logical engine IDs to runtime adapters. +// +// # Built-in Engines +// +// NewEngineCatalog registers the four built-in engines: claude, codex, copilot, gemini. +// Each EngineDefinition carries the engine's RuntimeID which maps to the corresponding +// CodingAgentEngine registered in the EngineRegistry. +// +// # Resolve() +// +// EngineCatalog.Resolve() performs: +// 1. Exact catalog ID lookup +// 2. Runtime-ID prefix fallback (for backward compat, e.g. "codex-experimental") +// 3. Formatted validation error when engine is unknown +package workflow + +import ( + "fmt" + "strings" + + "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/parser" +) + +// ProviderSelection identifies the AI provider for an engine (e.g. "anthropic", "openai"). +type ProviderSelection struct { + Name string +} + +// ModelSelection specifies the default and supported models for an engine. +type ModelSelection struct { + Default string + Supported []string +} + +// AuthBinding maps a logical authentication role to a secret name. +type AuthBinding struct { + Role string + Secret string +} + +// EngineDefinition holds the declarative metadata for an AI engine. +// It is separate from the runtime adapter (CodingAgentEngine) to allow the catalog +// layer to carry identity and provider information without coupling to implementation. +type EngineDefinition struct { + ID string + DisplayName string + Description string + RuntimeID string // maps to the CodingAgentEngine registered in EngineRegistry + Provider ProviderSelection + Models ModelSelection + Auth []AuthBinding + Options map[string]any +} + +// EngineCatalog is a collection of EngineDefinition entries backed by an EngineRegistry +// for runtime adapter resolution. +type EngineCatalog struct { + definitions map[string]*EngineDefinition + registry *EngineRegistry +} + +// ResolvedEngineTarget is the result of resolving an engine ID through the catalog. +// It combines the EngineDefinition, the caller-supplied EngineConfig, and the resolved +// CodingAgentEngine runtime adapter. +type ResolvedEngineTarget struct { + Definition *EngineDefinition + Config *EngineConfig // resolved merged config supplied by the caller + Runtime CodingAgentEngine // resolved adapter from the EngineRegistry +} + +// NewEngineCatalog creates an EngineCatalog that wraps the given EngineRegistry and +// pre-registers the four built-in engine definitions (claude, codex, copilot, gemini). +func NewEngineCatalog(registry *EngineRegistry) *EngineCatalog { + catalog := &EngineCatalog{ + definitions: make(map[string]*EngineDefinition), + registry: registry, + } + + catalog.Register(&EngineDefinition{ + ID: "claude", + DisplayName: "Claude Code", + Description: "Uses Claude Code with full MCP tool support and allow-listing", + RuntimeID: "claude", + Provider: ProviderSelection{Name: "anthropic"}, + Auth: []AuthBinding{ + {Role: "api-key", Secret: "ANTHROPIC_API_KEY"}, + }, + }) + + catalog.Register(&EngineDefinition{ + ID: "codex", + DisplayName: "Codex", + Description: "Uses OpenAI Codex CLI with MCP server support", + RuntimeID: "codex", + Provider: ProviderSelection{Name: "openai"}, + Auth: []AuthBinding{ + {Role: "api-key", Secret: "CODEX_API_KEY"}, + }, + }) + + catalog.Register(&EngineDefinition{ + ID: "copilot", + DisplayName: "GitHub Copilot CLI", + Description: "Uses GitHub Copilot CLI with MCP server support", + RuntimeID: "copilot", + Provider: ProviderSelection{Name: "github"}, + }) + + catalog.Register(&EngineDefinition{ + ID: "gemini", + DisplayName: "Google Gemini CLI", + Description: "Google Gemini CLI with headless mode and LLM gateway support", + RuntimeID: "gemini", + Provider: ProviderSelection{Name: "google"}, + }) + + return catalog +} + +// Register adds or replaces an EngineDefinition in the catalog. +func (c *EngineCatalog) Register(def *EngineDefinition) { + c.definitions[def.ID] = def +} + +// Resolve returns a ResolvedEngineTarget for the given engine ID and config. +// Resolution order: +// 1. Exact match in the catalog by ID +// 2. Prefix match in the underlying EngineRegistry (backward compat, e.g. "codex-experimental") +// 3. Returns a formatted validation error when no match is found +func (c *EngineCatalog) Resolve(id string, config *EngineConfig) (*ResolvedEngineTarget, error) { + // Exact catalog lookup + if def, ok := c.definitions[id]; ok { + runtime, err := c.registry.GetEngine(def.RuntimeID) + if err != nil { + return nil, fmt.Errorf("engine %q definition references unknown runtime %q: %w", id, def.RuntimeID, err) + } + return &ResolvedEngineTarget{Definition: def, Config: config, Runtime: runtime}, nil + } + + // Fall back to runtime-ID prefix lookup for backward compat (e.g. "codex-experimental") + runtime, err := c.registry.GetEngineByPrefix(id) + if err == nil { + def := &EngineDefinition{ + ID: id, + DisplayName: runtime.GetDisplayName(), + Description: runtime.GetDescription(), + RuntimeID: runtime.GetID(), + } + return &ResolvedEngineTarget{Definition: def, Config: config, Runtime: runtime}, nil + } + + // Engine not found — produce a helpful validation error matching the existing format + validEngines := c.registry.GetSupportedEngines() + suggestions := parser.FindClosestMatches(id, validEngines, 1) + enginesStr := strings.Join(validEngines, ", ") + + errMsg := fmt.Sprintf("invalid engine: %s. Valid engines are: %s.\n\nExample:\nengine: copilot\n\nSee: %s", + id, + enginesStr, + constants.DocsEnginesURL) + + if len(suggestions) > 0 { + errMsg = fmt.Sprintf("invalid engine: %s. Valid engines are: %s.\n\nDid you mean: %s?\n\nExample:\nengine: copilot\n\nSee: %s", + id, + enginesStr, + suggestions[0], + constants.DocsEnginesURL) + } + + return nil, fmt.Errorf("%s", errMsg) +} diff --git a/pkg/workflow/engine_definition_test.go b/pkg/workflow/engine_definition_test.go new file mode 100644 index 00000000000..9d060807c23 --- /dev/null +++ b/pkg/workflow/engine_definition_test.go @@ -0,0 +1,130 @@ +//go:build !integration + +package workflow + +import ( + "testing" + + "github.com/github/gh-aw/pkg/constants" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestNewEngineCatalog_BuiltIns checks that all four built-in engines are registered +// and resolve to the expected runtime adapters. +func TestNewEngineCatalog_BuiltIns(t *testing.T) { + registry := NewEngineRegistry() + catalog := NewEngineCatalog(registry) + + tests := []struct { + engineID string + displayName string + provider string + }{ + {"claude", "Claude Code", "anthropic"}, + {"codex", "Codex", "openai"}, + {"copilot", "GitHub Copilot CLI", "github"}, + {"gemini", "Google Gemini CLI", "google"}, + } + + for _, tt := range tests { + t.Run(tt.engineID, func(t *testing.T) { + resolved, err := catalog.Resolve(tt.engineID, &EngineConfig{ID: tt.engineID}) + require.NoError(t, err, "expected %s to resolve without error", tt.engineID) + require.NotNil(t, resolved, "expected non-nil ResolvedEngineTarget for %s", tt.engineID) + + assert.Equal(t, tt.engineID, resolved.Definition.ID, "Definition.ID should match") + assert.Equal(t, tt.displayName, resolved.Definition.DisplayName, "Definition.DisplayName should match") + assert.Equal(t, tt.provider, resolved.Definition.Provider.Name, "Definition.Provider.Name should match") + assert.Equal(t, tt.engineID, resolved.Runtime.GetID(), "Runtime.GetID() should match engine ID") + }) + } +} + +// TestEngineCatalog_Resolve_LegacyStringFormat verifies that resolving via plain string +// ("engine: claude") and object ID format ("engine.id: claude") produce the same runtime. +func TestEngineCatalog_Resolve_LegacyStringFormat(t *testing.T) { + registry := NewEngineRegistry() + catalog := NewEngineCatalog(registry) + + // Simulate "engine: claude" — EngineConfig built from string + stringConfig := &EngineConfig{ID: "claude"} + resolvedString, err := catalog.Resolve("claude", stringConfig) + require.NoError(t, err, "string-format engine should resolve without error") + + // Simulate "engine:\n id: claude" — same logical ID + objectConfig := &EngineConfig{ID: "claude"} + resolvedObject, err := catalog.Resolve("claude", objectConfig) + require.NoError(t, err, "object-format engine should resolve without error") + + assert.Equal(t, resolvedString.Runtime.GetID(), resolvedObject.Runtime.GetID(), + "both formats should resolve to the same runtime adapter") + assert.Equal(t, resolvedString.Definition.ID, resolvedObject.Definition.ID, + "both formats should resolve to the same definition") +} + +// TestEngineCatalog_Resolve_PrefixFallback verifies backward-compat prefix matching +// (e.g. "codex-experimental" should resolve to the codex runtime). +func TestEngineCatalog_Resolve_PrefixFallback(t *testing.T) { + registry := NewEngineRegistry() + catalog := NewEngineCatalog(registry) + + resolved, err := catalog.Resolve("codex-experimental", &EngineConfig{ID: "codex-experimental"}) + require.NoError(t, err, "prefix-matched engine should resolve without error") + require.NotNil(t, resolved, "expected non-nil ResolvedEngineTarget for prefix match") + + assert.Equal(t, "codex", resolved.Runtime.GetID(), "prefix match should resolve to codex runtime") +} + +// TestEngineCatalog_Resolve_UnknownEngine verifies that unknown engine IDs return a +// descriptive validation error containing the engine ID, a list of valid engines, and +// the documentation URL. +func TestEngineCatalog_Resolve_UnknownEngine(t *testing.T) { + registry := NewEngineRegistry() + catalog := NewEngineCatalog(registry) + + _, err := catalog.Resolve("nonexistent-engine", &EngineConfig{ID: "nonexistent-engine"}) + require.Error(t, err, "unknown engine should return an error") + assert.Contains(t, err.Error(), "invalid engine", + "error should mention 'invalid engine', got: %s", err.Error()) + assert.Contains(t, err.Error(), "nonexistent-engine", + "error should mention the unknown engine ID, got: %s", err.Error()) + assert.Contains(t, err.Error(), string(constants.DocsEnginesURL), + "error should include the engines documentation URL, got: %s", err.Error()) + assert.Contains(t, err.Error(), "engine: copilot", + "error should include an example, got: %s", err.Error()) +} + +// TestEngineCatalog_Resolve_ConfigPassthrough verifies that the EngineConfig passed to +// Resolve is surfaced unchanged in the ResolvedEngineTarget. +func TestEngineCatalog_Resolve_ConfigPassthrough(t *testing.T) { + registry := NewEngineRegistry() + catalog := NewEngineCatalog(registry) + + cfg := &EngineConfig{ID: "copilot", Model: "gpt-4o", MaxTurns: "10"} + resolved, err := catalog.Resolve("copilot", cfg) + require.NoError(t, err, "copilot with config should resolve without error") + assert.Equal(t, cfg, resolved.Config, "resolved Config should be the same pointer passed in") +} + +// TestEngineCatalog_Register_Custom verifies that a custom engine definition can be +// registered and resolved via the catalog. +func TestEngineCatalog_Register_Custom(t *testing.T) { + registry := NewEngineRegistry() + // Register a test engine in the registry so the catalog can look it up + registry.Register(NewCopilotEngine()) // reuse copilot as the backing runtime + + catalog := NewEngineCatalog(registry) + catalog.Register(&EngineDefinition{ + ID: "my-custom-engine", + DisplayName: "My Custom Engine", + Description: "A custom engine for testing", + RuntimeID: "copilot", // backed by copilot runtime + Provider: ProviderSelection{Name: "custom"}, + }) + + resolved, err := catalog.Resolve("my-custom-engine", &EngineConfig{ID: "my-custom-engine"}) + require.NoError(t, err, "custom engine should resolve without error") + assert.Equal(t, "my-custom-engine", resolved.Definition.ID, "custom engine definition ID should match") + assert.Equal(t, "copilot", resolved.Runtime.GetID(), "custom engine should use copilot runtime") +}