Skip to content

Commit a185bcf

Browse files
authored
Phase 1: Introduce EngineDefinition, EngineCatalog, and ResolvedEngineTarget (#20459)
1 parent 231a9f8 commit a185bcf

File tree

4 files changed

+325
-10
lines changed

4 files changed

+325
-10
lines changed

pkg/workflow/compiler_orchestrator_engine.go

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -199,19 +199,17 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean
199199
}
200200
}
201201

202-
// Validate the engine setting
203-
orchestratorEngineLog.Printf("Validating engine setting: %s", engineSetting)
204-
if err := c.validateEngine(engineSetting); err != nil {
205-
orchestratorEngineLog.Printf("Engine validation failed: %v", err)
206-
return nil, err
207-
}
208-
209-
// Get the agentic engine instance
210-
agenticEngine, err := c.getAgenticEngine(engineSetting)
202+
// Validate the engine setting and resolve the runtime adapter via the catalog.
203+
// This performs exact catalog lookup, prefix fallback, and returns a formatted
204+
// validation error for unknown engines — replacing the separate validateEngine
205+
// and getAgenticEngine calls.
206+
orchestratorEngineLog.Printf("Resolving engine setting: %s", engineSetting)
207+
resolvedEngine, err := c.engineCatalog.Resolve(engineSetting, engineConfig)
211208
if err != nil {
212-
orchestratorEngineLog.Printf("Failed to get agentic engine: %v", err)
209+
orchestratorEngineLog.Printf("Engine resolution failed: %v", err)
213210
return nil, err
214211
}
212+
agenticEngine := resolvedEngine.Runtime
215213

216214
log.Printf("AI engine: %s (%s)", agenticEngine.GetDisplayName(), engineSetting)
217215
if agenticEngine.IsExperimental() && c.verbose {

pkg/workflow/compiler_types.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ type Compiler struct {
9898
actionTag string // Override action SHA or tag for actions/setup (when set, overrides actionMode to release)
9999
jobManager *JobManager // Manages jobs and dependencies
100100
engineRegistry *EngineRegistry // Registry of available agentic engines
101+
engineCatalog *EngineCatalog // Catalog of engine definitions backed by the registry
101102
fileTracker FileTracker // Optional file tracker for tracking created files
102103
warningCount int // Number of warnings encountered during compilation
103104
stepOrderTracker *StepOrderTracker // Tracks step ordering for validation
@@ -136,6 +137,7 @@ func NewCompiler(opts ...CompilerOption) *Compiler {
136137
actionMode: DetectActionMode(version), // Auto-detect action mode based on version
137138
jobManager: NewJobManager(),
138139
engineRegistry: GetGlobalEngineRegistry(),
140+
engineCatalog: NewEngineCatalog(GetGlobalEngineRegistry()),
139141
stepOrderTracker: NewStepOrderTracker(),
140142
artifactManager: NewArtifactManager(),
141143
actionPinWarnings: make(map[string]bool), // Initialize warning cache

pkg/workflow/engine_definition.go

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
// This file defines the engine definition layer: declarative metadata types for AI engines,
2+
// a catalog of registered definitions, and a resolved target that combines definition,
3+
// config, and runtime adapter.
4+
//
5+
// # Architecture
6+
//
7+
// The engine definition layer sits on top of the existing EngineRegistry runtime layer:
8+
//
9+
// EngineDefinition – declarative metadata for a single engine entry
10+
// EngineCatalog – registry of EngineDefinition entries with a Resolve() method
11+
// ResolvedEngineTarget – result of resolving an engine ID: definition + config + runtime
12+
//
13+
// The existing EngineRegistry and CodingAgentEngine interfaces are unchanged; the catalog
14+
// is an additional layer that maps logical engine IDs to runtime adapters.
15+
//
16+
// # Built-in Engines
17+
//
18+
// NewEngineCatalog registers the four built-in engines: claude, codex, copilot, gemini.
19+
// Each EngineDefinition carries the engine's RuntimeID which maps to the corresponding
20+
// CodingAgentEngine registered in the EngineRegistry.
21+
//
22+
// # Resolve()
23+
//
24+
// EngineCatalog.Resolve() performs:
25+
// 1. Exact catalog ID lookup
26+
// 2. Runtime-ID prefix fallback (for backward compat, e.g. "codex-experimental")
27+
// 3. Formatted validation error when engine is unknown
28+
package workflow
29+
30+
import (
31+
"fmt"
32+
"strings"
33+
34+
"github.com/github/gh-aw/pkg/constants"
35+
"github.com/github/gh-aw/pkg/parser"
36+
)
37+
38+
// ProviderSelection identifies the AI provider for an engine (e.g. "anthropic", "openai").
39+
type ProviderSelection struct {
40+
Name string
41+
}
42+
43+
// ModelSelection specifies the default and supported models for an engine.
44+
type ModelSelection struct {
45+
Default string
46+
Supported []string
47+
}
48+
49+
// AuthBinding maps a logical authentication role to a secret name.
50+
type AuthBinding struct {
51+
Role string
52+
Secret string
53+
}
54+
55+
// EngineDefinition holds the declarative metadata for an AI engine.
56+
// It is separate from the runtime adapter (CodingAgentEngine) to allow the catalog
57+
// layer to carry identity and provider information without coupling to implementation.
58+
type EngineDefinition struct {
59+
ID string
60+
DisplayName string
61+
Description string
62+
RuntimeID string // maps to the CodingAgentEngine registered in EngineRegistry
63+
Provider ProviderSelection
64+
Models ModelSelection
65+
Auth []AuthBinding
66+
Options map[string]any
67+
}
68+
69+
// EngineCatalog is a collection of EngineDefinition entries backed by an EngineRegistry
70+
// for runtime adapter resolution.
71+
type EngineCatalog struct {
72+
definitions map[string]*EngineDefinition
73+
registry *EngineRegistry
74+
}
75+
76+
// ResolvedEngineTarget is the result of resolving an engine ID through the catalog.
77+
// It combines the EngineDefinition, the caller-supplied EngineConfig, and the resolved
78+
// CodingAgentEngine runtime adapter.
79+
type ResolvedEngineTarget struct {
80+
Definition *EngineDefinition
81+
Config *EngineConfig // resolved merged config supplied by the caller
82+
Runtime CodingAgentEngine // resolved adapter from the EngineRegistry
83+
}
84+
85+
// NewEngineCatalog creates an EngineCatalog that wraps the given EngineRegistry and
86+
// pre-registers the four built-in engine definitions (claude, codex, copilot, gemini).
87+
func NewEngineCatalog(registry *EngineRegistry) *EngineCatalog {
88+
catalog := &EngineCatalog{
89+
definitions: make(map[string]*EngineDefinition),
90+
registry: registry,
91+
}
92+
93+
catalog.Register(&EngineDefinition{
94+
ID: "claude",
95+
DisplayName: "Claude Code",
96+
Description: "Uses Claude Code with full MCP tool support and allow-listing",
97+
RuntimeID: "claude",
98+
Provider: ProviderSelection{Name: "anthropic"},
99+
Auth: []AuthBinding{
100+
{Role: "api-key", Secret: "ANTHROPIC_API_KEY"},
101+
},
102+
})
103+
104+
catalog.Register(&EngineDefinition{
105+
ID: "codex",
106+
DisplayName: "Codex",
107+
Description: "Uses OpenAI Codex CLI with MCP server support",
108+
RuntimeID: "codex",
109+
Provider: ProviderSelection{Name: "openai"},
110+
Auth: []AuthBinding{
111+
{Role: "api-key", Secret: "CODEX_API_KEY"},
112+
},
113+
})
114+
115+
catalog.Register(&EngineDefinition{
116+
ID: "copilot",
117+
DisplayName: "GitHub Copilot CLI",
118+
Description: "Uses GitHub Copilot CLI with MCP server support",
119+
RuntimeID: "copilot",
120+
Provider: ProviderSelection{Name: "github"},
121+
})
122+
123+
catalog.Register(&EngineDefinition{
124+
ID: "gemini",
125+
DisplayName: "Google Gemini CLI",
126+
Description: "Google Gemini CLI with headless mode and LLM gateway support",
127+
RuntimeID: "gemini",
128+
Provider: ProviderSelection{Name: "google"},
129+
})
130+
131+
return catalog
132+
}
133+
134+
// Register adds or replaces an EngineDefinition in the catalog.
135+
func (c *EngineCatalog) Register(def *EngineDefinition) {
136+
c.definitions[def.ID] = def
137+
}
138+
139+
// Resolve returns a ResolvedEngineTarget for the given engine ID and config.
140+
// Resolution order:
141+
// 1. Exact match in the catalog by ID
142+
// 2. Prefix match in the underlying EngineRegistry (backward compat, e.g. "codex-experimental")
143+
// 3. Returns a formatted validation error when no match is found
144+
func (c *EngineCatalog) Resolve(id string, config *EngineConfig) (*ResolvedEngineTarget, error) {
145+
// Exact catalog lookup
146+
if def, ok := c.definitions[id]; ok {
147+
runtime, err := c.registry.GetEngine(def.RuntimeID)
148+
if err != nil {
149+
return nil, fmt.Errorf("engine %q definition references unknown runtime %q: %w", id, def.RuntimeID, err)
150+
}
151+
return &ResolvedEngineTarget{Definition: def, Config: config, Runtime: runtime}, nil
152+
}
153+
154+
// Fall back to runtime-ID prefix lookup for backward compat (e.g. "codex-experimental")
155+
runtime, err := c.registry.GetEngineByPrefix(id)
156+
if err == nil {
157+
def := &EngineDefinition{
158+
ID: id,
159+
DisplayName: runtime.GetDisplayName(),
160+
Description: runtime.GetDescription(),
161+
RuntimeID: runtime.GetID(),
162+
}
163+
return &ResolvedEngineTarget{Definition: def, Config: config, Runtime: runtime}, nil
164+
}
165+
166+
// Engine not found — produce a helpful validation error matching the existing format
167+
validEngines := c.registry.GetSupportedEngines()
168+
suggestions := parser.FindClosestMatches(id, validEngines, 1)
169+
enginesStr := strings.Join(validEngines, ", ")
170+
171+
errMsg := fmt.Sprintf("invalid engine: %s. Valid engines are: %s.\n\nExample:\nengine: copilot\n\nSee: %s",
172+
id,
173+
enginesStr,
174+
constants.DocsEnginesURL)
175+
176+
if len(suggestions) > 0 {
177+
errMsg = fmt.Sprintf("invalid engine: %s. Valid engines are: %s.\n\nDid you mean: %s?\n\nExample:\nengine: copilot\n\nSee: %s",
178+
id,
179+
enginesStr,
180+
suggestions[0],
181+
constants.DocsEnginesURL)
182+
}
183+
184+
return nil, fmt.Errorf("%s", errMsg)
185+
}
Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
//go:build !integration
2+
3+
package workflow
4+
5+
import (
6+
"testing"
7+
8+
"github.com/github/gh-aw/pkg/constants"
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
// TestNewEngineCatalog_BuiltIns checks that all four built-in engines are registered
14+
// and resolve to the expected runtime adapters.
15+
func TestNewEngineCatalog_BuiltIns(t *testing.T) {
16+
registry := NewEngineRegistry()
17+
catalog := NewEngineCatalog(registry)
18+
19+
tests := []struct {
20+
engineID string
21+
displayName string
22+
provider string
23+
}{
24+
{"claude", "Claude Code", "anthropic"},
25+
{"codex", "Codex", "openai"},
26+
{"copilot", "GitHub Copilot CLI", "github"},
27+
{"gemini", "Google Gemini CLI", "google"},
28+
}
29+
30+
for _, tt := range tests {
31+
t.Run(tt.engineID, func(t *testing.T) {
32+
resolved, err := catalog.Resolve(tt.engineID, &EngineConfig{ID: tt.engineID})
33+
require.NoError(t, err, "expected %s to resolve without error", tt.engineID)
34+
require.NotNil(t, resolved, "expected non-nil ResolvedEngineTarget for %s", tt.engineID)
35+
36+
assert.Equal(t, tt.engineID, resolved.Definition.ID, "Definition.ID should match")
37+
assert.Equal(t, tt.displayName, resolved.Definition.DisplayName, "Definition.DisplayName should match")
38+
assert.Equal(t, tt.provider, resolved.Definition.Provider.Name, "Definition.Provider.Name should match")
39+
assert.Equal(t, tt.engineID, resolved.Runtime.GetID(), "Runtime.GetID() should match engine ID")
40+
})
41+
}
42+
}
43+
44+
// TestEngineCatalog_Resolve_LegacyStringFormat verifies that resolving via plain string
45+
// ("engine: claude") and object ID format ("engine.id: claude") produce the same runtime.
46+
func TestEngineCatalog_Resolve_LegacyStringFormat(t *testing.T) {
47+
registry := NewEngineRegistry()
48+
catalog := NewEngineCatalog(registry)
49+
50+
// Simulate "engine: claude" — EngineConfig built from string
51+
stringConfig := &EngineConfig{ID: "claude"}
52+
resolvedString, err := catalog.Resolve("claude", stringConfig)
53+
require.NoError(t, err, "string-format engine should resolve without error")
54+
55+
// Simulate "engine:\n id: claude" — same logical ID
56+
objectConfig := &EngineConfig{ID: "claude"}
57+
resolvedObject, err := catalog.Resolve("claude", objectConfig)
58+
require.NoError(t, err, "object-format engine should resolve without error")
59+
60+
assert.Equal(t, resolvedString.Runtime.GetID(), resolvedObject.Runtime.GetID(),
61+
"both formats should resolve to the same runtime adapter")
62+
assert.Equal(t, resolvedString.Definition.ID, resolvedObject.Definition.ID,
63+
"both formats should resolve to the same definition")
64+
}
65+
66+
// TestEngineCatalog_Resolve_PrefixFallback verifies backward-compat prefix matching
67+
// (e.g. "codex-experimental" should resolve to the codex runtime).
68+
func TestEngineCatalog_Resolve_PrefixFallback(t *testing.T) {
69+
registry := NewEngineRegistry()
70+
catalog := NewEngineCatalog(registry)
71+
72+
resolved, err := catalog.Resolve("codex-experimental", &EngineConfig{ID: "codex-experimental"})
73+
require.NoError(t, err, "prefix-matched engine should resolve without error")
74+
require.NotNil(t, resolved, "expected non-nil ResolvedEngineTarget for prefix match")
75+
76+
assert.Equal(t, "codex", resolved.Runtime.GetID(), "prefix match should resolve to codex runtime")
77+
}
78+
79+
// TestEngineCatalog_Resolve_UnknownEngine verifies that unknown engine IDs return a
80+
// descriptive validation error containing the engine ID, a list of valid engines, and
81+
// the documentation URL.
82+
func TestEngineCatalog_Resolve_UnknownEngine(t *testing.T) {
83+
registry := NewEngineRegistry()
84+
catalog := NewEngineCatalog(registry)
85+
86+
_, err := catalog.Resolve("nonexistent-engine", &EngineConfig{ID: "nonexistent-engine"})
87+
require.Error(t, err, "unknown engine should return an error")
88+
assert.Contains(t, err.Error(), "invalid engine",
89+
"error should mention 'invalid engine', got: %s", err.Error())
90+
assert.Contains(t, err.Error(), "nonexistent-engine",
91+
"error should mention the unknown engine ID, got: %s", err.Error())
92+
assert.Contains(t, err.Error(), string(constants.DocsEnginesURL),
93+
"error should include the engines documentation URL, got: %s", err.Error())
94+
assert.Contains(t, err.Error(), "engine: copilot",
95+
"error should include an example, got: %s", err.Error())
96+
}
97+
98+
// TestEngineCatalog_Resolve_ConfigPassthrough verifies that the EngineConfig passed to
99+
// Resolve is surfaced unchanged in the ResolvedEngineTarget.
100+
func TestEngineCatalog_Resolve_ConfigPassthrough(t *testing.T) {
101+
registry := NewEngineRegistry()
102+
catalog := NewEngineCatalog(registry)
103+
104+
cfg := &EngineConfig{ID: "copilot", Model: "gpt-4o", MaxTurns: "10"}
105+
resolved, err := catalog.Resolve("copilot", cfg)
106+
require.NoError(t, err, "copilot with config should resolve without error")
107+
assert.Equal(t, cfg, resolved.Config, "resolved Config should be the same pointer passed in")
108+
}
109+
110+
// TestEngineCatalog_Register_Custom verifies that a custom engine definition can be
111+
// registered and resolved via the catalog.
112+
func TestEngineCatalog_Register_Custom(t *testing.T) {
113+
registry := NewEngineRegistry()
114+
// Register a test engine in the registry so the catalog can look it up
115+
registry.Register(NewCopilotEngine()) // reuse copilot as the backing runtime
116+
117+
catalog := NewEngineCatalog(registry)
118+
catalog.Register(&EngineDefinition{
119+
ID: "my-custom-engine",
120+
DisplayName: "My Custom Engine",
121+
Description: "A custom engine for testing",
122+
RuntimeID: "copilot", // backed by copilot runtime
123+
Provider: ProviderSelection{Name: "custom"},
124+
})
125+
126+
resolved, err := catalog.Resolve("my-custom-engine", &EngineConfig{ID: "my-custom-engine"})
127+
require.NoError(t, err, "custom engine should resolve without error")
128+
assert.Equal(t, "my-custom-engine", resolved.Definition.ID, "custom engine definition ID should match")
129+
assert.Equal(t, "copilot", resolved.Runtime.GetID(), "custom engine should use copilot runtime")
130+
}

0 commit comments

Comments
 (0)