From 69566b2268a779fc0d72ca2176bce326f6642700 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 03:43:44 +0000 Subject: [PATCH 1/2] Initial plan From aaebc850dd8c6473d008e87bfc342dd51b58cff4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 03:58:08 +0000 Subject: [PATCH 2/2] feat: add EngineCatalog helper methods, fix gemini drift, add drift-detection test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_interactive_engine.go | 20 +++-- pkg/constants/constants.go | 21 ++++- pkg/constants/constants_test.go | 5 +- pkg/workflow/engine_catalog_test.go | 120 ++++++++++++++++++++++++++++ pkg/workflow/engine_definition.go | 31 +++++++ 5 files changed, 185 insertions(+), 12 deletions(-) create mode 100644 pkg/workflow/engine_catalog_test.go diff --git a/pkg/cli/add_interactive_engine.go b/pkg/cli/add_interactive_engine.go index 34ab71efc47..888a246bc5f 100644 --- a/pkg/cli/add_interactive_engine.go +++ b/pkg/cli/add_interactive_engine.go @@ -7,6 +7,7 @@ import ( "github.com/charmbracelet/huh" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/workflow" ) // selectAIEngineAndKey prompts the user to select an AI engine and provide API key @@ -80,20 +81,25 @@ func (c *AddInteractiveConfig) selectAIEngineAndKey() error { fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Workflow specifies engine: "+workflowSpecifiedEngine)) } - // Build engine options with notes about existing secrets and workflow specification + // Build engine options with notes about existing secrets and workflow specification. + // The list of engines is derived from the catalog to ensure all registered engines appear. + catalog := workflow.NewEngineCatalog(workflow.NewEngineRegistry()) var engineOptions []huh.Option[string] - for _, opt := range constants.EngineOptions { - label := fmt.Sprintf("%s - %s", opt.Label, opt.Description) - // Add markers for secret availability and workflow specification - if c.existingSecrets[opt.SecretName] { + for _, def := range catalog.All() { + opt := constants.GetEngineOption(def.ID) + label := fmt.Sprintf("%s - %s", def.DisplayName, def.Description) + // Add markers for secret availability and workflow specification. + // opt may be nil for catalog engines not yet represented in EngineOptions; + // in that case we conservatively show '[no secret]'. + if opt != nil && c.existingSecrets[opt.SecretName] { label += " [secret exists]" } else { label += " [no secret]" } - if opt.Value == workflowSpecifiedEngine { + if def.ID == workflowSpecifiedEngine { label += " [specified in workflow]" } - engineOptions = append(engineOptions, huh.NewOption(label, opt.Value)) + engineOptions = append(engineOptions, huh.NewOption(label, def.ID)) } var selectedEngine string diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index 00fb81f4018..3ef5cd02e46 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -676,9 +676,11 @@ const ( GeminiEngine EngineName = "gemini" ) -// AgenticEngines lists all supported agentic engine names -// Note: This remains a string slice for backward compatibility with existing code -var AgenticEngines = []string{string(ClaudeEngine), string(CodexEngine), string(CopilotEngine)} +// AgenticEngines lists all supported agentic engine names. +// Deprecated: Use workflow.NewEngineCatalog(workflow.NewEngineRegistry()).IDs() for a +// catalog-derived list. This slice is maintained for backward compatibility and must +// stay in sync with the built-in engines registered in NewEngineCatalog. +var AgenticEngines = []string{string(ClaudeEngine), string(CodexEngine), string(CopilotEngine), string(GeminiEngine)} // EngineOption represents a selectable AI engine with its display metadata and secret configuration type EngineOption struct { @@ -692,7 +694,10 @@ type EngineOption struct { WhenNeeded string // Human-readable description of when this secret is needed } -// EngineOptions provides the list of available AI engines for user selection +// EngineOptions provides the list of available AI engines for user selection. +// Each entry includes secret metadata used by the interactive add wizard. +// Must stay in sync with the built-in engines registered in workflow.NewEngineCatalog; +// the TestEngineCatalogMatchesSchema test in pkg/workflow catches catalog/schema drift. var EngineOptions = []EngineOption{ { Value: string(CopilotEngine), @@ -720,6 +725,14 @@ var EngineOptions = []EngineOption{ KeyURL: "https://platform.openai.com/api-keys", WhenNeeded: "Codex/OpenAI engine workflows", }, + { + Value: string(GeminiEngine), + Label: "Gemini", + Description: "Google Gemini CLI coding agent", + SecretName: "GEMINI_API_KEY", + KeyURL: "https://aistudio.google.com/app/apikey", + WhenNeeded: "Gemini engine workflows", + }, } // SystemSecretSpec describes a system-level secret that is not engine-specific diff --git a/pkg/constants/constants_test.go b/pkg/constants/constants_test.go index b606aa55905..d716a739f56 100644 --- a/pkg/constants/constants_test.go +++ b/pkg/constants/constants_test.go @@ -83,7 +83,7 @@ func TestAgenticEngines(t *testing.T) { t.Error("AgenticEngines should not be empty") } - expectedEngines := []string{"claude", "codex", "copilot"} + expectedEngines := []string{"claude", "codex", "copilot", "gemini"} if len(AgenticEngines) != len(expectedEngines) { t.Errorf("AgenticEngines length = %d, want %d", len(AgenticEngines), len(expectedEngines)) } @@ -104,6 +104,9 @@ func TestAgenticEngines(t *testing.T) { if string(CopilotEngine) != "copilot" { t.Errorf("CopilotEngine constant = %q, want %q", CopilotEngine, "copilot") } + if string(GeminiEngine) != "gemini" { + t.Errorf("GeminiEngine constant = %q, want %q", GeminiEngine, "gemini") + } } func TestDefaultGitHubTools(t *testing.T) { diff --git a/pkg/workflow/engine_catalog_test.go b/pkg/workflow/engine_catalog_test.go new file mode 100644 index 00000000000..b2f6849f6a9 --- /dev/null +++ b/pkg/workflow/engine_catalog_test.go @@ -0,0 +1,120 @@ +//go:build !integration + +package workflow + +import ( + "encoding/json" + "os" + "sort" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestEngineCatalog_IDs verifies that IDs() returns all engine IDs in sorted order. +func TestEngineCatalog_IDs(t *testing.T) { + registry := NewEngineRegistry() + catalog := NewEngineCatalog(registry) + + ids := catalog.IDs() + require.NotEmpty(t, ids, "IDs() should return a non-empty list") + + // Verify all built-in engines are present + expectedIDs := []string{"claude", "codex", "copilot", "gemini"} + assert.Equal(t, expectedIDs, ids, "IDs() should return all built-in engines in sorted order") + + // Verify the list is sorted + sorted := make([]string, len(ids)) + copy(sorted, ids) + sort.Strings(sorted) + assert.Equal(t, sorted, ids, "IDs() should return IDs in sorted order") +} + +// TestEngineCatalog_DisplayNames verifies that DisplayNames() returns names in sorted ID order. +func TestEngineCatalog_DisplayNames(t *testing.T) { + registry := NewEngineRegistry() + catalog := NewEngineCatalog(registry) + + names := catalog.DisplayNames() + require.NotEmpty(t, names, "DisplayNames() should return a non-empty list") + assert.Len(t, names, len(catalog.IDs()), "DisplayNames() should have same length as IDs()") + + // Verify display names match expected values in sorted ID order (claude, codex, copilot, gemini) + expectedNames := []string{"Claude Code", "Codex", "GitHub Copilot CLI", "Google Gemini CLI"} + assert.Equal(t, expectedNames, names, "DisplayNames() should return display names in sorted ID order") +} + +// TestEngineCatalog_All verifies that All() returns all definitions in sorted ID order. +func TestEngineCatalog_All(t *testing.T) { + registry := NewEngineRegistry() + catalog := NewEngineCatalog(registry) + + defs := catalog.All() + require.NotEmpty(t, defs, "All() should return a non-empty list") + assert.Len(t, defs, len(catalog.IDs()), "All() should have same length as IDs()") + + ids := catalog.IDs() + for i, def := range defs { + assert.Equal(t, ids[i], def.ID, "All()[%d].ID should match IDs()[%d]", i, i) + assert.NotEmpty(t, def.DisplayName, "All()[%d].DisplayName should not be empty", i) + } +} + +// engineSchemaEnums parses the main workflow schema and extracts engine enum values +// from both the string variant and the object id property of engine_config. +func engineSchemaEnums(t *testing.T) []string { + t.Helper() + + schemaBytes, err := os.ReadFile("../parser/schemas/main_workflow_schema.json") + require.NoError(t, err, "should be able to read main_workflow_schema.json") + + var schema map[string]any + require.NoError(t, json.Unmarshal(schemaBytes, &schema), "schema should be valid JSON") + + defs, ok := schema["$defs"].(map[string]any) + require.True(t, ok, "schema should have $defs") + + engineConfig, ok := defs["engine_config"].(map[string]any) + require.True(t, ok, "$defs should have engine_config") + + oneOf, ok := engineConfig["oneOf"].([]any) + require.True(t, ok, "engine_config should have oneOf") + + // The first oneOf variant is the plain string enum + for _, variant := range oneOf { + v, ok := variant.(map[string]any) + if !ok { + continue + } + if v["type"] == "string" { + rawEnum, ok := v["enum"].([]any) + if !ok { + continue + } + enums := make([]string, 0, len(rawEnum)) + for _, e := range rawEnum { + if s, ok := e.(string); ok { + enums = append(enums, s) + } + } + sort.Strings(enums) + return enums + } + } + t.Fatal("could not find string enum in engine_config oneOf") + return nil +} + +// TestEngineCatalogMatchesSchema asserts that the schema engine enum values exactly +// match the catalog IDs. A failure here means the schema and catalog have drifted apart. +func TestEngineCatalogMatchesSchema(t *testing.T) { + registry := NewEngineRegistry() + catalog := NewEngineCatalog(registry) + + catalogIDs := catalog.IDs() // already sorted + schemaEnums := engineSchemaEnums(t) + + assert.Equal(t, catalogIDs, schemaEnums, + "schema engine enum must match catalog IDs exactly — run 'make build' after updating the schema") +} diff --git a/pkg/workflow/engine_definition.go b/pkg/workflow/engine_definition.go index e3154ad2cb1..bba72c58466 100644 --- a/pkg/workflow/engine_definition.go +++ b/pkg/workflow/engine_definition.go @@ -29,6 +29,7 @@ package workflow import ( "fmt" + "sort" "strings" "github.com/github/gh-aw/pkg/constants" @@ -136,6 +137,36 @@ func (c *EngineCatalog) Register(def *EngineDefinition) { c.definitions[def.ID] = def } +// IDs returns a sorted list of all engine IDs in the catalog. +func (c *EngineCatalog) IDs() []string { + ids := make([]string, 0, len(c.definitions)) + for id := range c.definitions { + ids = append(ids, id) + } + sort.Strings(ids) + return ids +} + +// DisplayNames returns a list of engine display names in sorted ID order. +func (c *EngineCatalog) DisplayNames() []string { + ids := c.IDs() + names := make([]string, 0, len(ids)) + for _, id := range ids { + names = append(names, c.definitions[id].DisplayName) + } + return names +} + +// All returns all engine definitions in sorted ID order. +func (c *EngineCatalog) All() []*EngineDefinition { + ids := c.IDs() + defs := make([]*EngineDefinition, 0, len(ids)) + for _, id := range ids { + defs = append(defs, c.definitions[id]) + } + return defs +} + // Resolve returns a ResolvedEngineTarget for the given engine ID and config. // Resolution order: // 1. Exact match in the catalog by ID