diff --git a/docs/adr/41674-deduplicate-engine-env-assembly-and-codemod-migration-helpers.md b/docs/adr/41674-deduplicate-engine-env-assembly-and-codemod-migration-helpers.md new file mode 100644 index 00000000000..e1f173d4c51 --- /dev/null +++ b/docs/adr/41674-deduplicate-engine-env-assembly-and-codemod-migration-helpers.md @@ -0,0 +1,45 @@ +# ADR-41674: Deduplicate Engine Env Assembly and Codemod Migration Helpers + +**Date**: 2026-06-26 +**Status**: Draft +**Deciders**: Unknown (automated refactor by copilot-swe-agent) + +--- + +### Context + +The three agentic engine implementations (Claude, Codex, and Copilot) each contained near-identical ~20-line inline blocks responsible for setting tool-timeout env vars (`GH_AW_STARTUP_TIMEOUT`, `GH_AW_TOOL_TIMEOUT`), max-turns env resolution (`GH_AW_MAX_TURNS`), engine/agent env merges, and MCP-scripts secret passthrough. Similarly, two codemods for migrating deprecated `engine.max-runs` and `engine.max-turns` to top-level YAML keys shared ~80 lines of near-identical frontmatter transformation logic. Any change to env-assembly semantics — such as adding a new runtime env var — required the same edit in three separate engine files. The `FormatStepWithCommandAndEnv` function also used a bespoke ad-hoc sort instead of the existing `sliceutil.SortedKeys` utility. + +### Decision + +We will extract the repeated env-assembly logic into four shared helper functions in `pkg/workflow/engine_helpers.go` (`applyOptionalEngineToolTimeouts`, `applyEngineMaxTurnsEnv`, `applyEngineAndAgentEnv`, `applyMCPScriptsSecretEnv`) and introduce a single `migrateEngineFieldToTopLevel` helper in `pkg/cli/codemod_engine_to_top_level_helpers.go` that both codemods delegate to. We will also replace the ad-hoc key-sort in `FormatStepWithCommandAndEnv` with `sliceutil.SortedKeys`. This centralises shared behavior without changing runtime semantics or user-visible output. + +### Alternatives Considered + +#### Alternative 1: Keep Inline Per-Engine Logic (Status Quo) + +Each engine file retains its own copy of the env-assembly blocks. The approach is maximally self-contained: reading any single engine file gives the full picture of what env vars it sets. This was rejected because the duplication made it error-prone to keep the three engines in sync when semantics change, as demonstrated by the three identical blocks already being the dominant maintenance burden called out in the PR description. + +#### Alternative 2: Interface-Based Polymorphism + +Define an `EngineEnvApplier` interface with an `ApplyCommonEnv(env map[string]string, wd *WorkflowData)` method, then have each engine embed a common implementation. This is a valid Go pattern for shared behavior across types, but it adds interface indirection for what is essentially stateless, side-effecting helper logic with no need for substitutability. Package-level functions are simpler and sufficient here. + +### Consequences + +#### Positive +- Single source of truth for each env-assembly concern; future additions (e.g., a new `GH_AW_*` env var) require exactly one code change. +- Net reduction of ~160 lines of duplicated code across the three engine files and two codemod files. +- `FormatStepWithCommandAndEnv` uses the shared `sliceutil.SortedKeys` utility consistently with other callers in the codebase. +- `migrateEngineFieldToTopLevel` is parameterised and reusable for any future `engine.*` → top-level migration codemods. + +#### Negative +- New engine implementations must know to call the shared helpers; this implicit contract is not enforced by the type system. +- The generic `migrateEngineFieldToTopLevel` signature (9 parameters) is harder to read in isolation than each of the original self-contained codemods, increasing cognitive overhead for understanding a single migration path. + +#### Neutral +- The refactor is behavior-preserving: no change to env-var names, values, or precedence order; existing tests continue to exercise the same code paths via the new helper call sites. +- The `docs/adr/` directory convention is being adopted concurrently with this PR, so this ADR is the first to use the PR-number-as-ADR-number naming scheme. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/codemod_engine_max_runs.go b/pkg/cli/codemod_engine_max_runs.go index 8e7835c71d7..dc89159734b 100644 --- a/pkg/cli/codemod_engine_max_runs.go +++ b/pkg/cli/codemod_engine_max_runs.go @@ -1,10 +1,6 @@ package cli -import ( - "strings" - - "github.com/github/gh-aw/pkg/logger" -) +import "github.com/github/gh-aw/pkg/logger" var engineMaxRunsCodemodLog = logger.New("cli:codemod_engine_max_runs") @@ -17,83 +13,17 @@ func getEngineMaxRunsToTopLevelCodemod() Codemod { Description: "Moves deprecated 'engine.max-runs' to top-level 'max-turns' so AWF enforces invocation caps consistently across all engines.", IntroducedIn: "0.17.0", Apply: func(content string, frontmatter map[string]any) (string, bool, error) { - engineValue, hasEngine := frontmatter["engine"] - if !hasEngine { - return content, false, nil - } - engineMap, ok := engineValue.(map[string]any) - if !ok { - return content, false, nil - } - if _, hasMaxRuns := engineMap["max-runs"]; !hasMaxRuns { - return content, false, nil - } - - _, hasTopLevelMaxRuns := frontmatter["max-runs"] - _, hasTopLevelMaxTurns := frontmatter["max-turns"] - - return applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) { - for _, line := range lines { - trimmed := strings.TrimSpace(line) - if !isTopLevelKey(line) || !strings.HasPrefix(trimmed, "engine:") { - continue - } - inlineValue := strings.TrimSpace(strings.TrimPrefix(trimmed, "engine:")) - if strings.HasPrefix(inlineValue, "{") && strings.Contains(inlineValue, "max-runs:") { - engineMaxRunsCodemodLog.Print("Skipping engine.max-runs migration for inline-map engine syntax; migrate to top-level max-turns manually") - return lines, false - } - } - - maxRunsSuffix := "" - inEngineBlock := false - engineIndent := "" - for _, line := range lines { - trimmed := strings.TrimSpace(line) - if isTopLevelKey(line) && strings.HasPrefix(trimmed, "engine:") { - inEngineBlock = true - engineIndent = getIndentation(line) - continue - } - if inEngineBlock && len(trimmed) > 0 && !strings.HasPrefix(trimmed, "#") && len(getIndentation(line)) <= len(engineIndent) { - inEngineBlock = false - } - if inEngineBlock && strings.HasPrefix(trimmed, "max-runs:") { - parts := strings.SplitN(line, ":", 2) - if len(parts) == 2 { - maxRunsSuffix = parts[1] - } - break - } - } - - result, removed := removeFieldFromBlock(lines, "max-runs", "engine") - if !removed { - return lines, false - } - - if hasTopLevelMaxRuns || hasTopLevelMaxTurns { - engineMaxRunsCodemodLog.Print("Removed deprecated engine.max-runs (top-level max-runs/max-turns already present)") - return result, true - } - - insertAt := 0 - for i, line := range result { - if isTopLevelKey(line) && strings.HasPrefix(strings.TrimSpace(line), "engine:") { - insertAt = i - break - } - } - - maxRunsLine := "max-turns:" + maxRunsSuffix - withTopLevel := make([]string, 0, len(result)+1) - withTopLevel = append(withTopLevel, result[:insertAt]...) - withTopLevel = append(withTopLevel, maxRunsLine) - withTopLevel = append(withTopLevel, result[insertAt:]...) - - engineMaxRunsCodemodLog.Print("Migrated engine.max-runs to top-level max-turns") - return withTopLevel, true - }) + return migrateEngineFieldToTopLevel( + content, + frontmatter, + "max-runs", + "max-turns", + []string{"max-runs", "max-turns"}, + engineMaxRunsCodemodLog, + "Skipping engine.max-runs migration for inline-map engine syntax; migrate to top-level max-turns manually", + "Removed deprecated engine.max-runs (top-level max-runs/max-turns already present)", + "Migrated engine.max-runs to top-level max-turns", + ) }, } } diff --git a/pkg/cli/codemod_engine_max_turns.go b/pkg/cli/codemod_engine_max_turns.go index 48c1a3ffefa..53a4dd874f5 100644 --- a/pkg/cli/codemod_engine_max_turns.go +++ b/pkg/cli/codemod_engine_max_turns.go @@ -1,10 +1,6 @@ package cli -import ( - "strings" - - "github.com/github/gh-aw/pkg/logger" -) +import "github.com/github/gh-aw/pkg/logger" var engineMaxTurnsCodemodLog = logger.New("cli:codemod_engine_max_turns") @@ -17,82 +13,17 @@ func getEngineMaxTurnsToTopLevelCodemod() Codemod { Description: "Moves deprecated 'engine.max-turns' to top-level 'max-turns' so AWF enforces turn caps consistently across all agentic engines.", IntroducedIn: "0.68.4", Apply: func(content string, frontmatter map[string]any) (string, bool, error) { - engineValue, hasEngine := frontmatter["engine"] - if !hasEngine { - return content, false, nil - } - engineMap, ok := engineValue.(map[string]any) - if !ok { - return content, false, nil - } - if _, hasMaxTurns := engineMap["max-turns"]; !hasMaxTurns { - return content, false, nil - } - - _, hasTopLevelMaxTurns := frontmatter["max-turns"] - - return applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) { - for _, line := range lines { - trimmed := strings.TrimSpace(line) - if !isTopLevelKey(line) || !strings.HasPrefix(trimmed, "engine:") { - continue - } - inlineValue := strings.TrimSpace(strings.TrimPrefix(trimmed, "engine:")) - if strings.HasPrefix(inlineValue, "{") && strings.Contains(inlineValue, "max-turns:") { - engineMaxTurnsCodemodLog.Print("Skipping engine.max-turns migration for inline-map engine syntax; migrate to top-level max-turns manually") - return lines, false - } - } - - maxTurnsSuffix := "" - inEngineBlock := false - engineIndent := "" - for _, line := range lines { - trimmed := strings.TrimSpace(line) - if isTopLevelKey(line) && strings.HasPrefix(trimmed, "engine:") { - inEngineBlock = true - engineIndent = getIndentation(line) - continue - } - if inEngineBlock && len(trimmed) > 0 && !strings.HasPrefix(trimmed, "#") && len(getIndentation(line)) <= len(engineIndent) { - inEngineBlock = false - } - if inEngineBlock && strings.HasPrefix(trimmed, "max-turns:") { - parts := strings.SplitN(line, ":", 2) - if len(parts) == 2 { - maxTurnsSuffix = parts[1] - } - break - } - } - - result, removed := removeFieldFromBlock(lines, "max-turns", "engine") - if !removed { - return lines, false - } - - if hasTopLevelMaxTurns { - engineMaxTurnsCodemodLog.Print("Removed deprecated engine.max-turns (top-level max-turns already present)") - return result, true - } - - insertAt := 0 - for i, line := range result { - if isTopLevelKey(line) && strings.HasPrefix(strings.TrimSpace(line), "engine:") { - insertAt = i - break - } - } - - maxTurnsLine := "max-turns:" + maxTurnsSuffix - withTopLevel := make([]string, 0, len(result)+1) - withTopLevel = append(withTopLevel, result[:insertAt]...) - withTopLevel = append(withTopLevel, maxTurnsLine) - withTopLevel = append(withTopLevel, result[insertAt:]...) - - engineMaxTurnsCodemodLog.Print("Migrated engine.max-turns to top-level max-turns") - return withTopLevel, true - }) + return migrateEngineFieldToTopLevel( + content, + frontmatter, + "max-turns", + "max-turns", + []string{"max-turns"}, + engineMaxTurnsCodemodLog, + "Skipping engine.max-turns migration for inline-map engine syntax; migrate to top-level max-turns manually", + "Removed deprecated engine.max-turns (top-level max-turns already present)", + "Migrated engine.max-turns to top-level max-turns", + ) }, } } diff --git a/pkg/cli/codemod_engine_to_top_level_helpers.go b/pkg/cli/codemod_engine_to_top_level_helpers.go new file mode 100644 index 00000000000..7f96aa8080b --- /dev/null +++ b/pkg/cli/codemod_engine_to_top_level_helpers.go @@ -0,0 +1,109 @@ +package cli + +import ( + "strings" + + "github.com/github/gh-aw/pkg/logger" +) + +func migrateEngineFieldToTopLevel( + content string, + frontmatter map[string]any, + engineField string, + targetTopLevelField string, + preserveTopLevelFields []string, + log *logger.Logger, + skipInlineMessage string, + removedMessage string, + migratedMessage string, +) (string, bool, error) { + engineValue, hasEngine := frontmatter["engine"] + if !hasEngine { + return content, false, nil + } + engineMap, ok := engineValue.(map[string]any) + if !ok { + return content, false, nil + } + if _, hasEngineField := engineMap[engineField]; !hasEngineField { + return content, false, nil + } + + hasPreservedTopLevelField := false + for _, field := range preserveTopLevelFields { + if _, exists := frontmatter[field]; exists { + hasPreservedTopLevelField = true + break + } + } + + return applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) { + for _, line := range lines { + trimmed := strings.TrimSpace(line) + if !isTopLevelKey(line) || !strings.HasPrefix(trimmed, "engine:") { + continue + } + inlineValue := strings.TrimSpace(strings.TrimPrefix(trimmed, "engine:")) + if strings.HasPrefix(inlineValue, "{") && strings.Contains(inlineValue, engineField+":") { + if log != nil { + log.Print(skipInlineMessage) + } + return lines, false + } + } + + fieldSuffix := "" + inEngineBlock := false + engineIndent := "" + engineFieldPrefix := engineField + ":" + for _, line := range lines { + trimmed := strings.TrimSpace(line) + if isTopLevelKey(line) && strings.HasPrefix(trimmed, "engine:") { + inEngineBlock = true + engineIndent = getIndentation(line) + continue + } + if inEngineBlock && len(trimmed) > 0 && !strings.HasPrefix(trimmed, "#") && len(getIndentation(line)) <= len(engineIndent) { + inEngineBlock = false + } + if inEngineBlock && strings.HasPrefix(trimmed, engineFieldPrefix) { + parts := strings.SplitN(line, ":", 2) + if len(parts) == 2 { + fieldSuffix = parts[1] + } + break + } + } + + result, removed := removeFieldFromBlock(lines, engineField, "engine") + if !removed { + return lines, false + } + + if hasPreservedTopLevelField { + if log != nil { + log.Print(removedMessage) + } + return result, true + } + + insertAt := 0 + for i, line := range result { + if isTopLevelKey(line) && strings.HasPrefix(strings.TrimSpace(line), "engine:") { + insertAt = i + break + } + } + + topLevelLine := targetTopLevelField + ":" + fieldSuffix + withTopLevel := make([]string, 0, len(result)+1) + withTopLevel = append(withTopLevel, result[:insertAt]...) + withTopLevel = append(withTopLevel, topLevelLine) + withTopLevel = append(withTopLevel, result[insertAt:]...) + + if log != nil { + log.Print(migratedMessage) + } + return withTopLevel, true + }) +} diff --git a/pkg/workflow/claude_engine.go b/pkg/workflow/claude_engine.go index bc18599b1ae..bc6eed2a31c 100644 --- a/pkg/workflow/claude_engine.go +++ b/pkg/workflow/claude_engine.go @@ -445,23 +445,8 @@ func (e *ClaudeEngine) GetExecutionSteps(workflowData *WorkflowData, logFile str // Propagate W3C trace context so engine spans nest under the gh-aw.agent.setup span. applyTraceContextEnvToMap(env) - // Add GH_AW_STARTUP_TIMEOUT environment variable (in seconds) if startup-timeout is specified - // Supports both literal integers and GitHub Actions expressions (e.g. "${{ inputs.startup-timeout }}") - if workflowData.ToolsStartupTimeout != "" { - env["GH_AW_STARTUP_TIMEOUT"] = workflowData.ToolsStartupTimeout - } - - // Add GH_AW_TOOL_TIMEOUT environment variable (in seconds) if timeout is specified - // Supports both literal integers and GitHub Actions expressions (e.g. "${{ inputs.tool-timeout }}") - if workflowData.ToolsTimeout != "" { - env["GH_AW_TOOL_TIMEOUT"] = workflowData.ToolsTimeout - } - - if workflowData.EngineConfig != nil && workflowData.EngineConfig.MaxTurns != "" { - env["GH_AW_MAX_TURNS"] = workflowData.EngineConfig.MaxTurns - } else { - env["GH_AW_MAX_TURNS"] = compilerenv.BuildDefaultMaxTurnsExpression() - } + applyOptionalEngineToolTimeouts(env, workflowData) + applyEngineMaxTurnsEnv(env, workflowData) // Set the model environment variable. // When model is configured, use the native ANTHROPIC_MODEL env var - the Claude CLI reads it @@ -482,31 +467,9 @@ func (e *ClaudeEngine) GetExecutionSteps(workflowData *WorkflowData, logFile str } } - // Inject GH_AW_ENGINE_CWD when engine.cwd is configured. applyEngineCwdEnv(env, workflowData) - - // Add custom environment variables from engine config - if workflowData.EngineConfig != nil && len(workflowData.EngineConfig.Env) > 0 { - maps.Copy(env, workflowData.EngineConfig.Env) - } - - // Add custom environment variables from agent config - agentConfig := getAgentConfig(workflowData) - if agentConfig != nil && len(agentConfig.Env) > 0 { - maps.Copy(env, agentConfig.Env) - claudeLog.Printf("Added %d custom env vars from agent config", len(agentConfig.Env)) - } - - // Add mcp-scripts secrets to env for passthrough to MCP servers - if IsMCPScriptsEnabled(workflowData.MCPScripts) { - mcpScriptsSecrets := collectMCPScriptsSecrets(workflowData.MCPScripts) - for varName, secretExpr := range mcpScriptsSecrets { - // Only add if not already in env - if _, exists := env[varName]; !exists { - env[varName] = secretExpr - } - } - } + applyEngineAndAgentEnv(env, workflowData, claudeLog) + applyMCPScriptsSecretEnv(env, workflowData) // Generate the step for Claude CLI execution stepName := "Execute Claude Code CLI" diff --git a/pkg/workflow/codex_engine.go b/pkg/workflow/codex_engine.go index 286c5468c13..cbd94cdad72 100644 --- a/pkg/workflow/codex_engine.go +++ b/pkg/workflow/codex_engine.go @@ -443,23 +443,8 @@ mkdir -p "$CODEX_HOME/logs" maps.Copy(env, getGitIdentityEnvVars()) } - // Add GH_AW_STARTUP_TIMEOUT environment variable (in seconds) if startup-timeout is specified - // Supports both literal integers and GitHub Actions expressions (e.g. "${{ inputs.startup-timeout }}") - if workflowData.ToolsStartupTimeout != "" { - env["GH_AW_STARTUP_TIMEOUT"] = workflowData.ToolsStartupTimeout - } - - // Add GH_AW_TOOL_TIMEOUT environment variable (in seconds) if timeout is specified - // Supports both literal integers and GitHub Actions expressions (e.g. "${{ inputs.tool-timeout }}") - if workflowData.ToolsTimeout != "" { - env["GH_AW_TOOL_TIMEOUT"] = workflowData.ToolsTimeout - } - - if workflowData.EngineConfig != nil && workflowData.EngineConfig.MaxTurns != "" { - env["GH_AW_MAX_TURNS"] = workflowData.EngineConfig.MaxTurns - } else { - env["GH_AW_MAX_TURNS"] = compilerenv.BuildDefaultMaxTurnsExpression() - } + applyOptionalEngineToolTimeouts(env, workflowData) + applyEngineMaxTurnsEnv(env, workflowData) // Set the model environment variable. // Codex has no native model env var, so model selection always goes through @@ -473,31 +458,9 @@ mkdir -p "$CODEX_HOME/logs" env[modelEnvVar] = compilerenv.BuildModelOverrideExpression(modelEnvVar, compilerenv.DefaultModelCodex, constants.CodexDefaultModel) } - // Inject GH_AW_ENGINE_CWD when engine.cwd is configured. applyEngineCwdEnv(env, workflowData) - - // Add custom environment variables from engine config - if workflowData.EngineConfig != nil && len(workflowData.EngineConfig.Env) > 0 { - maps.Copy(env, workflowData.EngineConfig.Env) - } - - // Add custom environment variables from agent config - agentConfig := getAgentConfig(workflowData) - if agentConfig != nil && len(agentConfig.Env) > 0 { - maps.Copy(env, agentConfig.Env) - codexEngineLog.Printf("Added %d custom env vars from agent config", len(agentConfig.Env)) - } - - // Add mcp-scripts secrets to env for passthrough to MCP servers - if IsMCPScriptsEnabled(workflowData.MCPScripts) { - mcpScriptsSecrets := collectMCPScriptsSecrets(workflowData.MCPScripts) - for varName, secretExpr := range mcpScriptsSecrets { - // Only add if not already in env - if _, exists := env[varName]; !exists { - env[varName] = secretExpr - } - } - } + applyEngineAndAgentEnv(env, workflowData, codexEngineLog) + applyMCPScriptsSecretEnv(env, workflowData) // Generate the step for Codex execution stepName := "Execute Codex CLI" diff --git a/pkg/workflow/copilot_engine_execution.go b/pkg/workflow/copilot_engine_execution.go index cb4f821e664..c3a1106fd09 100644 --- a/pkg/workflow/copilot_engine_execution.go +++ b/pkg/workflow/copilot_engine_execution.go @@ -636,23 +636,8 @@ touch %s // Propagate W3C trace context so engine spans nest under the gh-aw.agent.setup span. applyTraceContextEnvToMap(env) - // Add GH_AW_STARTUP_TIMEOUT environment variable (in seconds) if startup-timeout is specified - // Supports both literal integers and GitHub Actions expressions (e.g. "${{ inputs.startup-timeout }}") - if workflowData.ToolsStartupTimeout != "" { - env["GH_AW_STARTUP_TIMEOUT"] = workflowData.ToolsStartupTimeout - } - - // Add GH_AW_TOOL_TIMEOUT environment variable (in seconds) if timeout is specified - // Supports both literal integers and GitHub Actions expressions (e.g. "${{ inputs.tool-timeout }}") - if workflowData.ToolsTimeout != "" { - env["GH_AW_TOOL_TIMEOUT"] = workflowData.ToolsTimeout - } - - if workflowData.EngineConfig != nil && workflowData.EngineConfig.MaxTurns != "" { - env["GH_AW_MAX_TURNS"] = workflowData.EngineConfig.MaxTurns - } else { - env["GH_AW_MAX_TURNS"] = compilerenv.BuildDefaultMaxTurnsExpression() - } + applyOptionalEngineToolTimeouts(env, workflowData) + applyEngineMaxTurnsEnv(env, workflowData) if workflowData.EngineConfig != nil && workflowData.EngineConfig.CopilotSDK { if workflowData.EngineConfig.MaxToolDenials != "" { @@ -680,17 +665,7 @@ touch %s // Inject GH_AW_ENGINE_CWD when engine.cwd is configured. applyEngineCwdEnv(env, workflowData) - // Add custom environment variables from engine config - if workflowData.EngineConfig != nil && len(workflowData.EngineConfig.Env) > 0 { - maps.Copy(env, workflowData.EngineConfig.Env) - } - - // Add custom environment variables from agent config - agentConfig := getAgentConfig(workflowData) - if agentConfig != nil && len(agentConfig.Env) > 0 { - maps.Copy(env, agentConfig.Env) - copilotExecLog.Printf("Added %d custom env vars from agent config", len(agentConfig.Env)) - } + applyEngineAndAgentEnv(env, workflowData, copilotExecLog) // Always inject the Copilot integration ID for agentic workflows after all env merges // so user-supplied env does not override this value. @@ -738,16 +713,7 @@ touch %s } } - // Add mcp-scripts secrets to env for passthrough to MCP servers - if IsMCPScriptsEnabled(workflowData.MCPScripts) { - mcpScriptsSecrets := collectMCPScriptsSecrets(workflowData.MCPScripts) - for varName, secretExpr := range mcpScriptsSecrets { - // Only add if not already in env - if _, exists := env[varName]; !exists { - env[varName] = secretExpr - } - } - } + applyMCPScriptsSecretEnv(env, workflowData) // Generate the step for Copilot CLI execution stepName := "Execute GitHub Copilot CLI" diff --git a/pkg/workflow/engine_helpers.go b/pkg/workflow/engine_helpers.go index ab44cd7ce8e..37a7389e0a9 100644 --- a/pkg/workflow/engine_helpers.go +++ b/pkg/workflow/engine_helpers.go @@ -31,12 +31,15 @@ package workflow import ( "fmt" + "maps" "regexp" - "sort" "strings" + "github.com/github/gh-aw/pkg/workflow/compilerenv" + "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/setutil" + "github.com/github/gh-aw/pkg/sliceutil" ) var engineHelpersLog = logger.New("workflow:engine_helpers") @@ -110,6 +113,61 @@ func applyEngineCwdEnv(env map[string]string, workflowData *WorkflowData) { env["GH_AW_ENGINE_CWD"] = workflowData.EngineConfig.Cwd } +// applyOptionalEngineToolTimeouts adds optional tool timeout environment variables. +func applyOptionalEngineToolTimeouts(env map[string]string, workflowData *WorkflowData) { + if workflowData == nil { + return + } + if workflowData.ToolsStartupTimeout != "" { + env["GH_AW_STARTUP_TIMEOUT"] = workflowData.ToolsStartupTimeout + } + if workflowData.ToolsTimeout != "" { + env["GH_AW_TOOL_TIMEOUT"] = workflowData.ToolsTimeout + } +} + +// applyEngineMaxTurnsEnv sets GH_AW_MAX_TURNS from engine.max-turns or the default expression. +func applyEngineMaxTurnsEnv(env map[string]string, workflowData *WorkflowData) { + if workflowData != nil && workflowData.EngineConfig != nil && workflowData.EngineConfig.MaxTurns != "" { + env["GH_AW_MAX_TURNS"] = workflowData.EngineConfig.MaxTurns + return + } + env["GH_AW_MAX_TURNS"] = compilerenv.BuildDefaultMaxTurnsExpression() +} + +// applyEngineAndAgentEnv merges custom environment variables from engine and agent configs. +func applyEngineAndAgentEnv(env map[string]string, workflowData *WorkflowData, log *logger.Logger) { + if workflowData == nil { + return + } + if workflowData.EngineConfig != nil && len(workflowData.EngineConfig.Env) > 0 { + maps.Copy(env, workflowData.EngineConfig.Env) + } + agentConfig := getAgentConfig(workflowData) + if agentConfig != nil && len(agentConfig.Env) > 0 { + maps.Copy(env, agentConfig.Env) + if log != nil { + log.Printf("Added %d custom env vars from agent config", len(agentConfig.Env)) + } + } +} + +// applyMCPScriptsSecretEnv appends mcp-scripts secrets unless already present. +func applyMCPScriptsSecretEnv(env map[string]string, workflowData *WorkflowData) { + if workflowData == nil { + return + } + if !IsMCPScriptsEnabled(workflowData.MCPScripts) { + return + } + mcpScriptsSecrets := collectMCPScriptsSecrets(workflowData.MCPScripts) + for varName, secretExpr := range mcpScriptsSecrets { + if _, exists := env[varName]; !exists { + env[varName] = secretExpr + } + } +} + // GenerateMultiSecretValidationStep creates a GitHub Actions step that validates at least one // of multiple secrets is available. // secretNames: slice of secret names to validate (e.g., []string{"CODEX_API_KEY", "OPENAI_API_KEY"}) @@ -239,14 +297,7 @@ func FormatStepWithCommandAndEnv(stepLines []string, command string, env map[str // Add environment variables if len(env) > 0 { stepLines = append(stepLines, " env:") - // Sort environment keys for consistent output - envKeys := make([]string, 0, len(env)) - for key := range env { - envKeys = append(envKeys, key) - } - sort.Strings(envKeys) - - for _, key := range envKeys { + for _, key := range sliceutil.SortedKeys(env) { value := env[key] stepLines = appendEnvVarLine(stepLines, key, value) }