diff --git a/.github/workflows/copilot-session-insights.lock.yml b/.github/workflows/copilot-session-insights.lock.yml index 67f2d7d1d66..8780b9929f1 100644 --- a/.github/workflows/copilot-session-insights.lock.yml +++ b/.github/workflows/copilot-session-insights.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"1a205bf3354b3cf4ede0f23f8443a2f0dd553ee0a66727371426742bf0733e68","body_hash":"2a96a21304b975c51504dd556731857febd942d37b53e1fcc5b42a29855ab23e","strict":true,"agent_id":"claude","engine_versions":{"claude":"2.1.223"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"1a205bf3354b3cf4ede0f23f8443a2f0dd553ee0a66727371426742bf0733e68","body_hash":"2a96a21304b975c51504dd556731857febd942d37b53e1fcc5b42a29855ab23e","strict":true,"agent_id":"claude","engine_versions":{"claude":"2.1.224"}} # gh-aw-manifest: {"version":1,"secrets":["ANTHROPIC_API_KEY","COPILOT_GITHUB_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GH_AW_OTEL_GRAFANA_AUTHORIZATION","GH_AW_OTEL_GRAFANA_ENDPOINT","GH_AW_OTEL_SENTRY_AUTHORIZATION","GH_AW_OTEL_SENTRY_ENDPOINT","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"3d3c42e5aac5ba805825da76410c181273ba90b1","version":"v7.0.1"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"820762786026740c76f36085b0efc47a31fe5020","version":"v7.0.0"},{"repo":"actions/setup-python","sha":"5fda3b95a4ea91299a34e894583c3862153e4b97","version":"v7.0.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.44","digest":"sha256:0d727725c737b58c7bdf51f640cffb928385ec46517e0917c7f1a02f1bada8b4","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.44@sha256:0d727725c737b58c7bdf51f640cffb928385ec46517e0917c7f1a02f1bada8b4"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.44","digest":"sha256:b50fbadba138f6e9aba94aca09711335c489bb3b15861220cb66f6092e042dc7","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.44@sha256:b50fbadba138f6e9aba94aca09711335c489bb3b15861220cb66f6092e042dc7"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.44","digest":"sha256:c064d15974f7c933ec7d3f7b4038f4fd203547b3154bdc821afd379144887eff","pinned_image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.44@sha256:c064d15974f7c933ec7d3f7b4038f4fd203547b3154bdc821afd379144887eff"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.44","digest":"sha256:83e48bbe12c634be8c228a576832fe45f66c529ac3659db92bddbcf2eeb6d627","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.44@sha256:83e48bbe12c634be8c228a576832fe45f66c529ac3659db92bddbcf2eeb6d627"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.8","digest":"sha256:38bbea36cdb46a3c9d04d1db05e672966f5239b431a2022eb35881688e5721d8","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.8@sha256:38bbea36cdb46a3c9d04d1db05e672966f5239b431a2022eb35881688e5721d8"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:0d9f1fb5fd6610c0ac1f5194a38e45a8a1e81f8a390d5142d8e4e6f26a4b3196","pinned_image":"ghcr.io/github/gh-aw-node@sha256:0d9f1fb5fd6610c0ac1f5194a38e45a8a1e81f8a390d5142d8e4e6f26a4b3196"},{"image":"ghcr.io/github/github-mcp-server:v1.8.0","digest":"sha256:d5a18c04b92714c309eb46a2305087e91a4dbd80420f6e462656699f95093520","pinned_image":"ghcr.io/github/github-mcp-server:v1.8.0@sha256:d5a18c04b92714c309eb46a2305087e91a4dbd80420f6e462656699f95093520"}]} # This file was automatically generated by gh-aw. DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # diff --git a/docs/adr/51216-centralize-engine-secret-validation-config.md b/docs/adr/51216-centralize-engine-secret-validation-config.md new file mode 100644 index 00000000000..315bf997a86 --- /dev/null +++ b/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.* diff --git a/pkg/workflow/awf_feature_flags_test.go b/pkg/workflow/awf_feature_flags_test.go index 938279b823f..d84a6d545f5 100644 --- a/pkg/workflow/awf_feature_flags_test.go +++ b/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) { diff --git a/pkg/workflow/behavior_defined_engine.go b/pkg/workflow/behavior_defined_engine.go index 5e20b414616..bee3735c76d 100644 --- a/pkg/workflow/behavior_defined_engine.go +++ b/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 { diff --git a/pkg/workflow/claude_engine.go b/pkg/workflow/claude_engine.go index 99a2374ec96..ffda9fbca9b 100644 --- a/pkg/workflow/claude_engine.go +++ b/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 diff --git a/pkg/workflow/codex_engine.go b/pkg/workflow/codex_engine.go index 65ec43e0496..0c0e135baa0 100644 --- a/pkg/workflow/codex_engine.go +++ b/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 { diff --git a/pkg/workflow/copilot_engine_installation.go b/pkg/workflow/copilot_engine_installation.go index 74571bc0971..4667c33b437 100644 --- a/pkg/workflow/copilot_engine_installation.go +++ b/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 diff --git a/pkg/workflow/engine_helpers.go b/pkg/workflow/engine_helpers.go index 18a60f8c1cb..661dcb62d8c 100644 --- a/pkg/workflow/engine_helpers.go +++ b/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 diff --git a/pkg/workflow/gemini_engine.go b/pkg/workflow/gemini_engine.go index ee50fb3e7dc..276b5813bc8 100644 --- a/pkg/workflow/gemini_engine.go +++ b/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 diff --git a/pkg/workflow/pi_engine.go b/pkg/workflow/pi_engine.go index 7df5f5a23bd..4b8bc0fe26b 100644 --- a/pkg/workflow/pi_engine.go +++ b/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. diff --git a/pkg/workflow/secret_validation_test.go b/pkg/workflow/secret_validation_test.go index e684eb1b868..7a9e50bab88 100644 --- a/pkg/workflow/secret_validation_test.go +++ b/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, diff --git a/pkg/workflow/universal_llm_consumer_engine.go b/pkg/workflow/universal_llm_consumer_engine.go index 24ff5e8dd2e..e1a5e148958 100644 --- a/pkg/workflow/universal_llm_consumer_engine.go +++ b/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) {