Refactor engine secret validation wrappers

docs/adr/51216-centralize-engine-secret-validation-config.md

@@ -0,0 +1,44 @@
+# ADR-51216: Centralize Engine Secret Validation via a Shared Config Helper
+
+**Date**: 2026-08-07
+**Status**: Draft
+**Deciders**: Unknown (automated draft — review before accepting)
+
+---
+
+### Context
+
+Each workflow engine in `pkg/workflow` implements a `GetSecretValidationStep` method. By PR #51216, at least seven implementations (Claude, Codex, Copilot, Gemini, Pi, behavior-defined, and the universal LLM consumer engine) each repeated the same guard-then-delegate pattern: check an optional skip predicate, check for an empty secret list, then call `BuildDefaultSecretValidationStep`. Only the skip condition, engine display name, documentation URL, and secret list source differed per engine. Duplicating this three-part structure in six-plus locations raises the risk that new auth modes (WIF, BYOK, provider-token fallbacks) are applied inconsistently when one engine is updated but others are not.
+
+### Decision
+
+We will introduce `EngineSecretValidationConfig` (a config struct holding `SecretNames`, `EngineName`, `DocsURL`, and an optional `Skip` predicate) and `BuildEngineSecretValidationStep` (a shared helper in `engine_helpers.go`) that applies the skip predicate, guards on an empty secret list, and delegates to `BuildDefaultSecretValidationStep`. All engine `GetSecretValidationStep` implementations will be migrated to call `BuildEngineSecretValidationStep` with an engine-specific config, keeping engine-specific skip logic encapsulated as a `func(*WorkflowData) bool` closure in each engine file.
+
+### Alternatives Considered
+
+#### Alternative 1: Status quo — leave per-engine wrappers unchanged
+
+Keep each engine's explicit if-guard plus `BuildDefaultSecretValidationStep` call as-is. This requires no new types or shared code and is fully transparent at each call site. It was rejected because any future change to the skip-or-delegate pattern (for example, adding a unified logging hook or a new auth mode) must be applied manually across seven-plus engine files, increasing the risk of behavioral drift.
+
+#### Alternative 2: Engine interface with a default validation implementation
+
+Define a new `SecretValidator` interface (or embed a default implementation via struct embedding) on the engine type, moving the validation logic into a shared base. This is a more complete object-oriented approach and would also consolidate other shared engine behaviors. It was rejected as over-engineering for this change: the variation across engines is limited to a single config object, so a lightweight config-and-helper pattern achieves the same consolidation at lower structural cost and with no interface breakage for existing engine implementations.
+
+### Consequences
+
+#### Positive
+- Eliminates six-plus instances of the duplicated skip-guard-then-delegate wrapper; the pattern now lives in one function that is unit-tested independently.
+- Adding a new engine or a new auth-skip condition (WIF, BYOK, etc.) requires only populating a `Skip` field on the config struct rather than replicating the three-step pattern.
+- Engine-specific skip predicates remain in each engine file, preserving locality of domain knowledge.
+
+#### Negative
+- `engine_helpers.go` gains a new exported type and function, widening the surface of the shared-helpers module that is already a common dependency.
+- Callers must follow one level of indirection (the `Skip` function pointer) to understand when validation is suppressed; the condition is no longer a plain if-statement at the call site.
+
+#### Neutral
+- Unit tests for `BuildEngineSecretValidationStep` (skip policy, empty-secret-list guard, rendered-step assertion) are added to `secret_validation_test.go`, independent of per-engine tests.
+- The underlying `BuildDefaultSecretValidationStep` function is unchanged; only its callers are updated.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/workflow/awf_feature_flags_test.go

@@ -3,8 +3,9 @@
 package workflow
 
 import (
-	"github.com/stretchr/testify/assert"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestAWFSupportsExcludeEnv(t *testing.T) {

pkg/workflow/behavior_defined_engine.go

@@ -153,14 +153,15 @@ func (e *BehaviorDefinedEngine) GetSecretValidationStep(workflowData *WorkflowDa
 		seen[binding.Secret] = struct{}{}
 		secrets = append(secrets, binding.Secret)
 	}
-	if len(secrets) == 0 {
-		return GitHubActionStep{}
-	}
 	documentationURL := ""
 	if behavior.Installation != nil {
 		documentationURL = behavior.Installation.DocumentationURL
 	}
-	return BuildDefaultSecretValidationStep(workflowData, secrets, e.definition.DisplayName, documentationURL)
+	return BuildEngineSecretValidationStep(workflowData, EngineSecretValidationConfig{
+		SecretNames: secrets,
+		EngineName:  e.definition.DisplayName,
+		DocsURL:     documentationURL,
+	})
 }
 
 func (e *BehaviorDefinedEngine) GetInstallationSteps(workflowData *WorkflowData) []GitHubActionStep {

pkg/workflow/claude_engine.go

@@ -84,16 +84,14 @@ func (e *ClaudeEngine) GetSupportedEnvVarKeys() []string {
 // Returns an empty step if custom command is specified or if Anthropic WIF is configured.
 func (e *ClaudeEngine) GetSecretValidationStep(workflowData *WorkflowData) GitHubActionStep {
 	provider := e.ResolveLLMProvider(workflowData)
-	if provider == LLMProviderAnthropic && isAnthropicWIF(workflowData) {
-		return GitHubActionStep{}
-	}
-	providerSecrets := llmProviderSecretNames(provider)
-	return BuildDefaultSecretValidationStep(
-		workflowData,
-		providerSecrets,
-		"Claude Code",
-		llmProviderDocsURL(provider),
-	)
+	return BuildEngineSecretValidationStep(workflowData, EngineSecretValidationConfig{
+		SecretNames: llmProviderSecretNames(provider),
+		EngineName:  "Claude Code",
+		DocsURL:     llmProviderDocsURL(provider),
+		Skip: func(workflowData *WorkflowData) bool {
+			return provider == LLMProviderAnthropic && isAnthropicWIF(workflowData)
+		},
+	})
 }
 
 // isAnthropicWIF returns true when the workflow is configured to use Anthropic

pkg/workflow/codex_engine.go

@@ -102,12 +102,11 @@ func (e *CodexEngine) GetSupportedEnvVarKeys() []string {
 // GetSecretValidationStep returns the secret validation step for the Codex engine.
 // Returns an empty step if custom command is specified.
 func (e *CodexEngine) GetSecretValidationStep(workflowData *WorkflowData) GitHubActionStep {
-	return BuildDefaultSecretValidationStep(
-		workflowData,
-		[]string{"CODEX_API_KEY", "OPENAI_API_KEY"},
-		"Codex",
-		"https://github.github.com/gh-aw/reference/engines/#openai-codex",
-	)
+	return BuildEngineSecretValidationStep(workflowData, EngineSecretValidationConfig{
+		SecretNames: []string{"CODEX_API_KEY", "OPENAI_API_KEY"},
+		EngineName:  "Codex",
+		DocsURL:     "https://github.github.com/gh-aw/reference/engines/#openai-codex",
+	})
 }
 
 func (e *CodexEngine) GetInstallationSteps(workflowData *WorkflowData) []GitHubActionStep {

pkg/workflow/copilot_engine_installation.go

@@ -63,22 +63,24 @@ func getWorkspaceCommandPrefixFor(config *EngineConfig) string {
 //     is not required for model routing).
 func (e *CopilotEngine) GetSecretValidationStep(workflowData *WorkflowData) GitHubActionStep {
 	provider := e.ResolveLLMProvider(workflowData)
-	if provider == LLMProviderGitHub && hasCopilotRequestsWritePermission(workflowData) {
-		copilotInstallLog.Print("Skipping secret validation step: permissions.copilot-requests=write enabled, using GitHub Actions token")
-		return GitHubActionStep{}
-	}
-	if engineEnvHasNonEmptyValue(workflowData, constants.CopilotProviderBaseURL) ||
-		engineEnvHasNonEmptyValue(workflowData, constants.CopilotProviderAPIKey) ||
-		engineEnvHasNonEmptyValue(workflowData, constants.CopilotProviderBearerToken) {
-		copilotInstallLog.Print("Skipping COPILOT_GITHUB_TOKEN validation: BYOK provider credentials are configured")
-		return GitHubActionStep{}
-	}
-	return BuildDefaultSecretValidationStep(
-		workflowData,
-		llmProviderSecretNames(provider),
-		"GitHub Copilot CLI",
-		llmProviderDocsURL(provider),
-	)
+	return BuildEngineSecretValidationStep(workflowData, EngineSecretValidationConfig{
+		SecretNames: llmProviderSecretNames(provider),
+		EngineName:  "GitHub Copilot CLI",
+		DocsURL:     llmProviderDocsURL(provider),
+		Skip: func(workflowData *WorkflowData) bool {
+			if provider == LLMProviderGitHub && hasCopilotRequestsWritePermission(workflowData) {
+				copilotInstallLog.Print("Skipping secret validation step: permissions.copilot-requests=write enabled, using GitHub Actions token")
+				return true
+			}
+			if engineEnvHasNonEmptyValue(workflowData, constants.CopilotProviderBaseURL) ||
+				engineEnvHasNonEmptyValue(workflowData, constants.CopilotProviderAPIKey) ||
+				engineEnvHasNonEmptyValue(workflowData, constants.CopilotProviderBearerToken) {
+				copilotInstallLog.Print("Skipping COPILOT_GITHUB_TOKEN validation: BYOK provider credentials are configured")
+				return true
+			}
+			return false
+		},
+	})
 }
 
 // GetSecretFailureMessage returns a Copilot-specific guidance message shown in the agentic

pkg/workflow/engine_helpers.go

@@ -251,6 +251,27 @@ func GenerateMultiSecretValidationStep(secretNames []string, engineName, docsURL
 	return GitHubActionStep(stepLines)
 }
 
+// EngineSecretValidationConfig describes how an engine validates its required
+// authentication secrets.
+type EngineSecretValidationConfig struct {
+	SecretNames []string
+	EngineName  string
+	DocsURL     string
+	Skip        func(*WorkflowData) bool
+}
+
+// BuildEngineSecretValidationStep applies an engine-specific skip policy and
+// delegates rendering to BuildDefaultSecretValidationStep.
+func BuildEngineSecretValidationStep(workflowData *WorkflowData, config EngineSecretValidationConfig) GitHubActionStep {
+	if config.Skip != nil && config.Skip(workflowData) {
+		return GitHubActionStep{}
+	}
+	if len(config.SecretNames) == 0 {
+		return GitHubActionStep{}
+	}
+	return BuildDefaultSecretValidationStep(workflowData, config.SecretNames, config.EngineName, config.DocsURL)
+}
+
 // BuildDefaultSecretValidationStep returns a secret validation step for the given engine
 // configuration, or an empty step when a custom command is specified. This consolidates
 // the common guard+delegate pattern shared across all engine GetSecretValidationStep

pkg/workflow/gemini_engine.go

@@ -91,15 +91,12 @@ func (e *GeminiEngine) GetSupportedEnvVarKeys() []string {
 // GetSecretValidationStep returns the secret validation step for the Gemini engine.
 // Returns an empty step if custom command is specified or if Google/Vertex WIF is configured.
 func (e *GeminiEngine) GetSecretValidationStep(workflowData *WorkflowData) GitHubActionStep {
-	if isGeminiVertexWIF(workflowData) {
-		return GitHubActionStep{}
-	}
-	return BuildDefaultSecretValidationStep(
-		workflowData,
-		[]string{"GEMINI_API_KEY"},
-		"Gemini CLI",
-		"https://geminicli.com/docs/get-started/authentication/",
-	)
+	return BuildEngineSecretValidationStep(workflowData, EngineSecretValidationConfig{
+		SecretNames: []string{"GEMINI_API_KEY"},
+		EngineName:  "Gemini CLI",
+		DocsURL:     "https://geminicli.com/docs/get-started/authentication/",
+		Skip:        isGeminiVertexWIF,
+	})
 }
 
 // isGeminiVertexWIF returns true when the workflow is configured to use Google

pkg/workflow/pi_engine.go

@@ -198,15 +198,11 @@ func (e *PiEngine) GetSupportedEnvVarKeys() []string {
 func (e *PiEngine) GetSecretValidationStep(workflowData *WorkflowData) GitHubActionStep {
 	backend := resolvePiBackend(workflowData)
 	profile := getUniversalLLMBackendProfile(backend, hasCopilotRequestsWritePermission(workflowData))
-	if len(profile.coreSecretNames) == 0 {
-		return GitHubActionStep{}
-	}
-	return BuildDefaultSecretValidationStep(
-		workflowData,
-		profile.coreSecretNames,
-		"Pi",
-		"https://github.github.com/gh-aw/reference/engines/#pi",
-	)
+	return BuildEngineSecretValidationStep(workflowData, EngineSecretValidationConfig{
+		SecretNames: profile.coreSecretNames,
+		EngineName:  "Pi",
+		DocsURL:     "https://github.github.com/gh-aw/reference/engines/#pi",
+	})
 }
 
 // GetInstallationSteps returns the GitHub Actions steps needed to install the Pi CLI.

pkg/workflow/secret_validation_test.go

@@ -436,6 +436,43 @@ func TestEngineSecretValidationSkippedWhenEnvironmentConfigured(t *testing.T) {
 	}
 }
 
+func TestBuildEngineSecretValidationStep(t *testing.T) {
+	t.Run("applies skip policy before rendering", func(t *testing.T) {
+		step := BuildEngineSecretValidationStep(&WorkflowData{}, EngineSecretValidationConfig{
+			SecretNames: []string{"COPILOT_GITHUB_TOKEN"},
+			EngineName:  "GitHub Copilot CLI",
+			DocsURL:     "https://github.github.com/gh-aw/reference/engines/#github-copilot-default",
+			Skip: func(*WorkflowData) bool {
+				return true
+			},
+		})
+
+		require.Empty(t, step, "expected skip policy to suppress validation step")
+	})
+
+	t.Run("skips empty secret list", func(t *testing.T) {
+		step := BuildEngineSecretValidationStep(&WorkflowData{}, EngineSecretValidationConfig{
+			EngineName: "Engine Without Secrets",
+			DocsURL:    "https://docs.example.com",
+		})
+
+		require.Empty(t, step, "expected empty secret list to suppress validation step")
+	})
+
+	t.Run("renders configured validation step", func(t *testing.T) {
+		step := BuildEngineSecretValidationStep(&WorkflowData{}, EngineSecretValidationConfig{
+			SecretNames: []string{"COPILOT_GITHUB_TOKEN"},
+			EngineName:  "GitHub Copilot CLI",
+			DocsURL:     "https://github.github.com/gh-aw/reference/engines/#github-copilot-default",
+		})
+
+		require.NotEmpty(t, step, "expected configured validation step")
+		stepContent := strings.Join(step, "\n")
+		assert.Contains(t, stepContent, "Validate COPILOT_GITHUB_TOKEN secret")
+		assert.Contains(t, stepContent, "COPILOT_GITHUB_TOKEN: ${{ secrets.COPILOT_GITHUB_TOKEN }}")
+	})
+}
+
 func TestBuildDefaultSecretValidationStepHandlesNilWorkflowData(t *testing.T) {
 	step := BuildDefaultSecretValidationStep(
 		nil,

pkg/workflow/universal_llm_consumer_engine.go

@@ -160,10 +160,11 @@ func extractToolsConfig(workflowData *WorkflowData) (*ToolsConfig, map[string]an
 func (e *UniversalLLMConsumerEngine) GetUniversalSecretValidationStep(workflowData *WorkflowData, engineName, docsURL string) GitHubActionStep {
 	backend := e.resolveBackend(workflowData)
 	profile := getUniversalLLMBackendProfile(backend, hasCopilotRequestsWritePermission(workflowData))
-	if len(profile.coreSecretNames) == 0 {
-		return GitHubActionStep{}
-	}
-	return BuildDefaultSecretValidationStep(workflowData, profile.coreSecretNames, engineName, docsURL)
+	return BuildEngineSecretValidationStep(workflowData, EngineSecretValidationConfig{
+		SecretNames: profile.coreSecretNames,
+		EngineName:  engineName,
+		DocsURL:     docsURL,
+	})
 }
 
 func (e *UniversalLLMConsumerEngine) ApplyUniversalProviderEnv(env map[string]string, workflowData *WorkflowData, firewallEnabled bool) {