Skip to content

Commit 44cb369

Browse files
authored
Phase 2: Make EngineCatalog the single source of truth for engine metadata (#20462)
1 parent ecda4d1 commit 44cb369

File tree

5 files changed

+185
-12
lines changed

5 files changed

+185
-12
lines changed

pkg/cli/add_interactive_engine.go

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"github.com/charmbracelet/huh"
88
"github.com/github/gh-aw/pkg/console"
99
"github.com/github/gh-aw/pkg/constants"
10+
"github.com/github/gh-aw/pkg/workflow"
1011
)
1112

1213
// selectAIEngineAndKey prompts the user to select an AI engine and provide API key
@@ -80,20 +81,25 @@ func (c *AddInteractiveConfig) selectAIEngineAndKey() error {
8081
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Workflow specifies engine: "+workflowSpecifiedEngine))
8182
}
8283

83-
// Build engine options with notes about existing secrets and workflow specification
84+
// Build engine options with notes about existing secrets and workflow specification.
85+
// The list of engines is derived from the catalog to ensure all registered engines appear.
86+
catalog := workflow.NewEngineCatalog(workflow.NewEngineRegistry())
8487
var engineOptions []huh.Option[string]
85-
for _, opt := range constants.EngineOptions {
86-
label := fmt.Sprintf("%s - %s", opt.Label, opt.Description)
87-
// Add markers for secret availability and workflow specification
88-
if c.existingSecrets[opt.SecretName] {
88+
for _, def := range catalog.All() {
89+
opt := constants.GetEngineOption(def.ID)
90+
label := fmt.Sprintf("%s - %s", def.DisplayName, def.Description)
91+
// Add markers for secret availability and workflow specification.
92+
// opt may be nil for catalog engines not yet represented in EngineOptions;
93+
// in that case we conservatively show '[no secret]'.
94+
if opt != nil && c.existingSecrets[opt.SecretName] {
8995
label += " [secret exists]"
9096
} else {
9197
label += " [no secret]"
9298
}
93-
if opt.Value == workflowSpecifiedEngine {
99+
if def.ID == workflowSpecifiedEngine {
94100
label += " [specified in workflow]"
95101
}
96-
engineOptions = append(engineOptions, huh.NewOption(label, opt.Value))
102+
engineOptions = append(engineOptions, huh.NewOption(label, def.ID))
97103
}
98104

99105
var selectedEngine string

pkg/constants/constants.go

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -676,9 +676,11 @@ const (
676676
GeminiEngine EngineName = "gemini"
677677
)
678678

679-
// AgenticEngines lists all supported agentic engine names
680-
// Note: This remains a string slice for backward compatibility with existing code
681-
var AgenticEngines = []string{string(ClaudeEngine), string(CodexEngine), string(CopilotEngine)}
679+
// AgenticEngines lists all supported agentic engine names.
680+
// Deprecated: Use workflow.NewEngineCatalog(workflow.NewEngineRegistry()).IDs() for a
681+
// catalog-derived list. This slice is maintained for backward compatibility and must
682+
// stay in sync with the built-in engines registered in NewEngineCatalog.
683+
var AgenticEngines = []string{string(ClaudeEngine), string(CodexEngine), string(CopilotEngine), string(GeminiEngine)}
682684

683685
// EngineOption represents a selectable AI engine with its display metadata and secret configuration
684686
type EngineOption struct {
@@ -692,7 +694,10 @@ type EngineOption struct {
692694
WhenNeeded string // Human-readable description of when this secret is needed
693695
}
694696

695-
// EngineOptions provides the list of available AI engines for user selection
697+
// EngineOptions provides the list of available AI engines for user selection.
698+
// Each entry includes secret metadata used by the interactive add wizard.
699+
// Must stay in sync with the built-in engines registered in workflow.NewEngineCatalog;
700+
// the TestEngineCatalogMatchesSchema test in pkg/workflow catches catalog/schema drift.
696701
var EngineOptions = []EngineOption{
697702
{
698703
Value: string(CopilotEngine),
@@ -720,6 +725,14 @@ var EngineOptions = []EngineOption{
720725
KeyURL: "https://platform.openai.com/api-keys",
721726
WhenNeeded: "Codex/OpenAI engine workflows",
722727
},
728+
{
729+
Value: string(GeminiEngine),
730+
Label: "Gemini",
731+
Description: "Google Gemini CLI coding agent",
732+
SecretName: "GEMINI_API_KEY",
733+
KeyURL: "https://aistudio.google.com/app/apikey",
734+
WhenNeeded: "Gemini engine workflows",
735+
},
723736
}
724737

725738
// SystemSecretSpec describes a system-level secret that is not engine-specific

pkg/constants/constants_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ func TestAgenticEngines(t *testing.T) {
8383
t.Error("AgenticEngines should not be empty")
8484
}
8585

86-
expectedEngines := []string{"claude", "codex", "copilot"}
86+
expectedEngines := []string{"claude", "codex", "copilot", "gemini"}
8787
if len(AgenticEngines) != len(expectedEngines) {
8888
t.Errorf("AgenticEngines length = %d, want %d", len(AgenticEngines), len(expectedEngines))
8989
}
@@ -104,6 +104,9 @@ func TestAgenticEngines(t *testing.T) {
104104
if string(CopilotEngine) != "copilot" {
105105
t.Errorf("CopilotEngine constant = %q, want %q", CopilotEngine, "copilot")
106106
}
107+
if string(GeminiEngine) != "gemini" {
108+
t.Errorf("GeminiEngine constant = %q, want %q", GeminiEngine, "gemini")
109+
}
107110
}
108111

109112
func TestDefaultGitHubTools(t *testing.T) {
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
//go:build !integration
2+
3+
package workflow
4+
5+
import (
6+
"encoding/json"
7+
"os"
8+
"sort"
9+
"testing"
10+
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
)
14+
15+
// TestEngineCatalog_IDs verifies that IDs() returns all engine IDs in sorted order.
16+
func TestEngineCatalog_IDs(t *testing.T) {
17+
registry := NewEngineRegistry()
18+
catalog := NewEngineCatalog(registry)
19+
20+
ids := catalog.IDs()
21+
require.NotEmpty(t, ids, "IDs() should return a non-empty list")
22+
23+
// Verify all built-in engines are present
24+
expectedIDs := []string{"claude", "codex", "copilot", "gemini"}
25+
assert.Equal(t, expectedIDs, ids, "IDs() should return all built-in engines in sorted order")
26+
27+
// Verify the list is sorted
28+
sorted := make([]string, len(ids))
29+
copy(sorted, ids)
30+
sort.Strings(sorted)
31+
assert.Equal(t, sorted, ids, "IDs() should return IDs in sorted order")
32+
}
33+
34+
// TestEngineCatalog_DisplayNames verifies that DisplayNames() returns names in sorted ID order.
35+
func TestEngineCatalog_DisplayNames(t *testing.T) {
36+
registry := NewEngineRegistry()
37+
catalog := NewEngineCatalog(registry)
38+
39+
names := catalog.DisplayNames()
40+
require.NotEmpty(t, names, "DisplayNames() should return a non-empty list")
41+
assert.Len(t, names, len(catalog.IDs()), "DisplayNames() should have same length as IDs()")
42+
43+
// Verify display names match expected values in sorted ID order (claude, codex, copilot, gemini)
44+
expectedNames := []string{"Claude Code", "Codex", "GitHub Copilot CLI", "Google Gemini CLI"}
45+
assert.Equal(t, expectedNames, names, "DisplayNames() should return display names in sorted ID order")
46+
}
47+
48+
// TestEngineCatalog_All verifies that All() returns all definitions in sorted ID order.
49+
func TestEngineCatalog_All(t *testing.T) {
50+
registry := NewEngineRegistry()
51+
catalog := NewEngineCatalog(registry)
52+
53+
defs := catalog.All()
54+
require.NotEmpty(t, defs, "All() should return a non-empty list")
55+
assert.Len(t, defs, len(catalog.IDs()), "All() should have same length as IDs()")
56+
57+
ids := catalog.IDs()
58+
for i, def := range defs {
59+
assert.Equal(t, ids[i], def.ID, "All()[%d].ID should match IDs()[%d]", i, i)
60+
assert.NotEmpty(t, def.DisplayName, "All()[%d].DisplayName should not be empty", i)
61+
}
62+
}
63+
64+
// engineSchemaEnums parses the main workflow schema and extracts engine enum values
65+
// from both the string variant and the object id property of engine_config.
66+
func engineSchemaEnums(t *testing.T) []string {
67+
t.Helper()
68+
69+
schemaBytes, err := os.ReadFile("../parser/schemas/main_workflow_schema.json")
70+
require.NoError(t, err, "should be able to read main_workflow_schema.json")
71+
72+
var schema map[string]any
73+
require.NoError(t, json.Unmarshal(schemaBytes, &schema), "schema should be valid JSON")
74+
75+
defs, ok := schema["$defs"].(map[string]any)
76+
require.True(t, ok, "schema should have $defs")
77+
78+
engineConfig, ok := defs["engine_config"].(map[string]any)
79+
require.True(t, ok, "$defs should have engine_config")
80+
81+
oneOf, ok := engineConfig["oneOf"].([]any)
82+
require.True(t, ok, "engine_config should have oneOf")
83+
84+
// The first oneOf variant is the plain string enum
85+
for _, variant := range oneOf {
86+
v, ok := variant.(map[string]any)
87+
if !ok {
88+
continue
89+
}
90+
if v["type"] == "string" {
91+
rawEnum, ok := v["enum"].([]any)
92+
if !ok {
93+
continue
94+
}
95+
enums := make([]string, 0, len(rawEnum))
96+
for _, e := range rawEnum {
97+
if s, ok := e.(string); ok {
98+
enums = append(enums, s)
99+
}
100+
}
101+
sort.Strings(enums)
102+
return enums
103+
}
104+
}
105+
t.Fatal("could not find string enum in engine_config oneOf")
106+
return nil
107+
}
108+
109+
// TestEngineCatalogMatchesSchema asserts that the schema engine enum values exactly
110+
// match the catalog IDs. A failure here means the schema and catalog have drifted apart.
111+
func TestEngineCatalogMatchesSchema(t *testing.T) {
112+
registry := NewEngineRegistry()
113+
catalog := NewEngineCatalog(registry)
114+
115+
catalogIDs := catalog.IDs() // already sorted
116+
schemaEnums := engineSchemaEnums(t)
117+
118+
assert.Equal(t, catalogIDs, schemaEnums,
119+
"schema engine enum must match catalog IDs exactly — run 'make build' after updating the schema")
120+
}

pkg/workflow/engine_definition.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ package workflow
2929

3030
import (
3131
"fmt"
32+
"sort"
3233
"strings"
3334

3435
"github.com/github/gh-aw/pkg/constants"
@@ -136,6 +137,36 @@ func (c *EngineCatalog) Register(def *EngineDefinition) {
136137
c.definitions[def.ID] = def
137138
}
138139

140+
// IDs returns a sorted list of all engine IDs in the catalog.
141+
func (c *EngineCatalog) IDs() []string {
142+
ids := make([]string, 0, len(c.definitions))
143+
for id := range c.definitions {
144+
ids = append(ids, id)
145+
}
146+
sort.Strings(ids)
147+
return ids
148+
}
149+
150+
// DisplayNames returns a list of engine display names in sorted ID order.
151+
func (c *EngineCatalog) DisplayNames() []string {
152+
ids := c.IDs()
153+
names := make([]string, 0, len(ids))
154+
for _, id := range ids {
155+
names = append(names, c.definitions[id].DisplayName)
156+
}
157+
return names
158+
}
159+
160+
// All returns all engine definitions in sorted ID order.
161+
func (c *EngineCatalog) All() []*EngineDefinition {
162+
ids := c.IDs()
163+
defs := make([]*EngineDefinition, 0, len(ids))
164+
for _, id := range ids {
165+
defs = append(defs, c.definitions[id])
166+
}
167+
return defs
168+
}
169+
139170
// Resolve returns a ResolvedEngineTarget for the given engine ID and config.
140171
// Resolution order:
141172
// 1. Exact match in the catalog by ID

0 commit comments

Comments
 (0)