Skip to content

refactor: extract shared CLI engine execution logic; make Pi backend resolution configurable #41921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Jun 27, 2026
+254 −249
Merged

refactor: extract shared CLI engine execution logic; make Pi backend resolution configurable #41921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a1a0c0f
refactor: extract shared CLI engine execution logic and configurable …
Copilot Jun 27, 2026
22cc0b7
docs(adr): add draft ADR-41921 for shared CLI engine execution logic …
github-actions[bot] Jun 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions docs/adr/41921-extract-shared-cli-engine-execution-logic.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# ADR-41921: Extract Shared CLI Engine Execution Logic into UniversalLLMConsumerEngine

**Date**: 2026-06-27
**Status**: Draft
**Deciders**: pelikhan, copilot-swe-agent

---

### Context

The `pkg/workflow` package hosts multiple agentic engine implementations (CrushEngine, OpenCodeEngine, PiEngine) that each compile to GitHub Actions workflow steps. CrushEngine and OpenCodeEngine had ~90 lines of nearly-identical `GetExecutionSteps` logic: building a CLI command with a `run` subcommand, handling firewall-aware AWF wrapping, injecting the standard set of AWF environment variables (prompt path, workspace, MCP config, safe-output, trace context, max-turns, model), and formatting the resulting GitHub Actions step. Maintaining two copies increased the risk of one engine receiving a bug fix or behaviour change that the other did not. Additionally, Pi's `resolvePiBackend` function special-cased the `github-copilot` provider alias with an inline `strings.EqualFold` check, making the alias pattern non-reusable for future engines.

### Decision

We will consolidate the duplicated `GetExecutionSteps` logic into a single `BuildCLIEngineExecutionSteps` method on `UniversalLLMConsumerEngine`, parameterised by a `UniversalCLIEngineExecutionConfig` struct. Engine-specific parameters (binary name, extra CLI flags, permissions config file, step name, model env var name, timestamp behaviour) are passed via the struct; the common firewall-aware command construction, env injection, and step formatting live once in the shared method. We will also extract Pi's `github-copilot` alias lookup into a new `resolveBackendWithAliases` helper that accepts a caller-supplied alias map, making the pattern available to any future engine without ad-hoc string comparisons.

### Alternatives Considered

#### Alternative 1: Keep duplication, fix both engines in lockstep

Each engine retains its own `GetExecutionSteps` implementation. Divergences are managed through code review discipline and cross-referencing comments. This avoids introducing an abstraction but perpetuates the maintenance burden: every future change to the execution pattern (e.g., a new env var, a firewall flag) must be applied in two places and reviewed for both. Given that the two implementations already diverged in minor ways (e.g., `WriteTimestamp` only in Crush), this approach has already shown its fragility and was rejected.

#### Alternative 2: Extract a package-level function rather than a method on UniversalLLMConsumerEngine

The shared logic could live in a standalone function `BuildCLIEngineExecutionSteps(e SomeInterface, workflowData, logFile, cfg)` instead of a method. This keeps the function decoupled from `UniversalLLMConsumerEngine` but requires defining an interface or passing the engine explicitly. Since all CLI engines already embed `UniversalLLMConsumerEngine` (which provides `ApplyUniversalProviderEnv`, `GetUniversalRequiredSecretNames`, etc.), making the shared logic a method on that struct is more idiomatic in Go and avoids a new interface. The method approach was chosen.

### Consequences

#### Positive
- Eliminates approximately 220 lines of duplicated code across CrushEngine and OpenCodeEngine; future engine additions can reuse the pattern with a single call.
- Bug fixes and behaviour changes to the standard execution path (env injection, firewall wrapping, step formatting) now apply to all engines simultaneously.
- `resolveBackendWithAliases` makes provider alias registration declarative and testable independently of any specific engine.
- `UniversalCLIEngineExecutionConfig` makes per-engine variation explicit and visible in one place rather than scattered across two large functions.

#### Negative
- `UniversalCLIEngineExecutionConfig` is a new public struct that must be kept in sync as the execution pattern evolves; adding a new field requires updating all callers.
- Engine-specific flags encoded as struct fields (e.g., `WriteTimestamp: true` for Crush, `false` for OpenCode) are less discoverable than inline conditionals; a reviewer must trace from the config struct back to `BuildCLIEngineExecutionSteps` to understand their effect.
- Engines that diverge significantly from the shared pattern in the future may need to partially bypass `BuildCLIEngineExecutionSteps`, creating awkward partial-use of the abstraction.

#### Neutral
- The `compilerenv` import moves from `crush_engine.go` and `opencode_engine.go` into `universal_llm_consumer_engine.go`; individual engine files become simpler.
- `GetUniversalRequiredSecretNames` is introduced as the method called inside `BuildCLIEngineExecutionSteps`, replacing the per-engine `GetRequiredSecretNames` calls; callers outside this path are unaffected.

---

*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*
140 changes: 10 additions & 130 deletions pkg/workflow/crush_engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@ package workflow

import (
"fmt"
"maps"

"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/workflow/compilerenv"
)

var crushLog = logger.New("workflow:crush_engine")
Expand Down Expand Up @@ -143,134 +141,16 @@ func (e *CrushEngine) GetExecutionSteps(workflowData *WorkflowData, logFile stri
crushLog.Printf("Generating execution steps for Crush engine: workflow=%s, firewall=%v",
workflowData.Name, isFirewallEnabled(workflowData))

var steps []GitHubActionStep

// Step 1: Write .crush.json config (permissions)
configStep := e.generateCrushConfigStep(workflowData)
steps = append(steps, configStep)

// Step 2: Build CLI arguments
var crushArgs []string

modelConfigured := workflowData.EngineConfig != nil && workflowData.EngineConfig.Model != ""

// Enable verbose logging for debugging in CI
crushArgs = append(crushArgs, "--verbose")

// Prompt from file (positional argument to `crush run`).
// Keep this outside shellJoinArgs so command substitution expands at runtime.
promptArg := "\"$(cat /tmp/gh-aw/aw-prompts/prompt.txt)\""

// Build command name
commandName := "crush"
if workflowData.EngineConfig != nil && workflowData.EngineConfig.Command != "" {
commandName = workflowData.EngineConfig.Command
}
crushCommand := fmt.Sprintf("%s run %s %s", commandName, shellJoinArgs(crushArgs), promptArg)
crushCommand = getWorkspaceCommandPrefixFor(workflowData.EngineConfig) + crushCommand

// AWF wrapping
firewallEnabled := isFirewallEnabled(workflowData)
var command string
if firewallEnabled {
// Resolve model for provider-specific domain allowlisting
model := ""
if modelConfigured {
model = workflowData.EngineConfig.Model
}
// Get allowed domains: prefer the pre-warmed cache on WorkflowData to avoid
// re-running the expensive map+sort operation. Note: crush uses model-specific
// domains; the cache is populated with the same model during compilation.
var allowedDomains string
if workflowData.CachedAllowedDomainsComputed {
allowedDomains = workflowData.CachedAllowedDomainsStr
} else {
// The model was validated by validateUniversalLLMConsumerModel before reaching here,
// so a malformed model (e.g. leading slash) must never occur. Panic is the correct
// response to an internal invariant violation.
allowedDomains = mustGetAllowedDomainsForEngineWithModel(
constants.CrushEngine,
model,
workflowData.NetworkPermissions,
workflowData.Tools,
workflowData.Runtimes,
)
}

npmPathSetup := GetNpmBinPathSetup()
crushCommandWithPath := fmt.Sprintf("%s && %s", npmPathSetup, crushCommand)
if mcpCLIPath := GetMCPCLIPathSetup(workflowData); mcpCLIPath != "" {
crushCommandWithPath = fmt.Sprintf("%s && %s", mcpCLIPath, crushCommandWithPath)
}

command = BuildAWFCommand(AWFCommandConfig{
EngineName: "crush",
EngineCommand: crushCommandWithPath,
LogFile: logFile,
WorkflowData: workflowData,
UsesTTY: false,
AllowedDomains: allowedDomains,
})
} else {
command = fmt.Sprintf("set -o pipefail\nprintf '%%s' \"$(date +%%s%%3N)\" > %s\n%s 2>&1 | tee -a %s", AgentCLIStartMsPath, crushCommand, logFile)
}

env := map[string]string{
"GH_AW_PROMPT": constants.AwPromptsFile,
"GITHUB_WORKSPACE": "${{ github.workspace }}",
"RUNNER_TEMP": "${{ runner.temp }}",
"NO_PROXY": "localhost,127.0.0.1",
}
injectWorkflowCallNetworkAllowedEnv(env, workflowData)
e.ApplyUniversalProviderEnv(env, workflowData, firewallEnabled)

// MCP config path
if HasMCPServers(workflowData) {
env["GH_AW_MCP_CONFIG"] = "${{ github.workspace }}/.crush.json"
}

// Safe outputs env
applySafeOutputEnvToMap(env, workflowData)

// Propagate W3C trace context so engine spans nest under the gh-aw.agent.setup span.
applyTraceContextEnvToMap(env)

if workflowData.EngineConfig != nil && workflowData.EngineConfig.MaxTurns != "" {
env["GH_AW_MAX_TURNS"] = workflowData.EngineConfig.MaxTurns
} else {
env["GH_AW_MAX_TURNS"] = compilerenv.BuildDefaultMaxTurnsExpression()
}

// Model env var (only when explicitly configured)
if modelConfigured {
crushLog.Printf("Setting %s env var for model: %s",
constants.CrushCLIModelEnvVar, workflowData.EngineConfig.Model)
env[constants.CrushCLIModelEnvVar] = workflowData.EngineConfig.Model
}

// Custom env from engine config (allows provider override)
applyEngineCwdEnv(env, workflowData)
if workflowData.EngineConfig != nil && len(workflowData.EngineConfig.Env) > 0 {
maps.Copy(env, workflowData.EngineConfig.Env)
}

// Agent config env
agentConfig := getAgentConfig(workflowData)
if agentConfig != nil && len(agentConfig.Env) > 0 {
maps.Copy(env, agentConfig.Env)
}

// Build execution step
stepLines := []string{
" - name: Execute Crush CLI",
" id: agentic_execution",
}
allowedSecrets := e.GetRequiredSecretNames(workflowData)
filteredEnv := FilterEnvForSecrets(env, allowedSecrets)
stepLines = FormatStepWithCommandAndEnv(stepLines, command, filteredEnv)

steps = append(steps, GitHubActionStep(stepLines))
return steps
return e.BuildCLIEngineExecutionSteps(workflowData, logFile, UniversalCLIEngineExecutionConfig{
EngineConstant: constants.CrushEngine,
DefaultCommandName: "crush",
ExtraCLIArgs: []string{"--verbose"},
MCPConfigFile: ".crush.json",
StepName: "Execute Crush CLI",
ConfigStep: e.generateCrushConfigStep(workflowData),
ModelEnvVarName: constants.CrushCLIModelEnvVar,
WriteTimestamp: true,
})
}

// generateCrushConfigStep writes .crush.json with all permissions set to allow
Expand Down
124 changes: 10 additions & 114 deletions pkg/workflow/opencode_engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@ package workflow

import (
"fmt"
"maps"

"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/workflow/compilerenv"
)

var openCodeLog = logger.New("workflow:opencode_engine")
Expand Down Expand Up @@ -110,118 +108,16 @@ func (e *OpenCodeEngine) GetExecutionSteps(workflowData *WorkflowData, logFile s
openCodeLog.Printf("Generating execution steps for OpenCode engine: workflow=%s, firewall=%v",
workflowData.Name, isFirewallEnabled(workflowData))

var steps []GitHubActionStep

configStep := e.generateOpenCodeConfigStep(workflowData)
steps = append(steps, configStep)

var openCodeArgs []string
modelConfigured := workflowData.EngineConfig != nil && workflowData.EngineConfig.Model != ""

openCodeArgs = append(openCodeArgs, "--print-logs", "--log-level", "DEBUG")
promptArg := fmt.Sprintf("\"$(cat %s)\"", constants.AwPromptsFile)

commandName := "opencode"
if workflowData.EngineConfig != nil && workflowData.EngineConfig.Command != "" {
commandName = workflowData.EngineConfig.Command
}
openCodeCommand := fmt.Sprintf("%s run %s %s", commandName, shellJoinArgs(openCodeArgs), promptArg)
openCodeCommand = getWorkspaceCommandPrefixFor(workflowData.EngineConfig) + openCodeCommand

firewallEnabled := isFirewallEnabled(workflowData)
var command string
if firewallEnabled {
model := ""
if modelConfigured {
model = workflowData.EngineConfig.Model
}
// Get allowed domains: prefer the pre-warmed cache on WorkflowData to avoid
// re-running the expensive map+sort operation. Note: opencode uses model-specific
// domains; the cache is populated with the same model during compilation.
var allowedDomains string
if workflowData.CachedAllowedDomainsComputed {
allowedDomains = workflowData.CachedAllowedDomainsStr
} else {
// The model was validated by validateUniversalLLMConsumerModel before reaching here,
// so a malformed model (e.g. leading slash) must never occur. Panic is the correct
// response to an internal invariant violation.
allowedDomains = mustGetAllowedDomainsForEngineWithModel(
constants.OpenCodeEngine,
model,
workflowData.NetworkPermissions,
workflowData.Tools,
workflowData.Runtimes,
)
}

npmPathSetup := GetNpmBinPathSetup()
openCodeCommandWithPath := fmt.Sprintf("%s && %s", npmPathSetup, openCodeCommand)
if mcpCLIPath := GetMCPCLIPathSetup(workflowData); mcpCLIPath != "" {
openCodeCommandWithPath = fmt.Sprintf("%s && %s", mcpCLIPath, openCodeCommandWithPath)
}

command = BuildAWFCommand(AWFCommandConfig{
EngineName: "opencode",
EngineCommand: openCodeCommandWithPath,
LogFile: logFile,
WorkflowData: workflowData,
UsesTTY: false,
AllowedDomains: allowedDomains,
})
} else {
command = fmt.Sprintf("set -o pipefail\n%s 2>&1 | tee -a %s", openCodeCommand, logFile)
}

env := map[string]string{
"GH_AW_PROMPT": constants.AwPromptsFile,
"GITHUB_WORKSPACE": "${{ github.workspace }}",
"RUNNER_TEMP": "${{ runner.temp }}",
"NO_PROXY": "localhost,127.0.0.1",
}
injectWorkflowCallNetworkAllowedEnv(env, workflowData)
e.ApplyUniversalProviderEnv(env, workflowData, firewallEnabled)

if HasMCPServers(workflowData) {
env["GH_AW_MCP_CONFIG"] = "${{ github.workspace }}/opencode.jsonc"
}

applySafeOutputEnvToMap(env, workflowData)

// Propagate W3C trace context so engine spans nest under the gh-aw.agent.setup span.
applyTraceContextEnvToMap(env)

if workflowData.EngineConfig != nil && workflowData.EngineConfig.MaxTurns != "" {
env["GH_AW_MAX_TURNS"] = workflowData.EngineConfig.MaxTurns
} else {
env["GH_AW_MAX_TURNS"] = compilerenv.BuildDefaultMaxTurnsExpression()
}

if modelConfigured {
openCodeLog.Printf("Setting %s env var for model: %s",
constants.OpenCodeCLIModelEnvVar, workflowData.EngineConfig.Model)
env[constants.OpenCodeCLIModelEnvVar] = workflowData.EngineConfig.Model
}

applyEngineCwdEnv(env, workflowData)
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)
}

stepLines := []string{
" - name: Execute OpenCode CLI",
" id: agentic_execution",
}
allowedSecrets := e.GetRequiredSecretNames(workflowData)
filteredEnv := FilterEnvForSecrets(env, allowedSecrets)
stepLines = FormatStepWithCommandAndEnv(stepLines, command, filteredEnv)

steps = append(steps, GitHubActionStep(stepLines))
return steps
return e.BuildCLIEngineExecutionSteps(workflowData, logFile, UniversalCLIEngineExecutionConfig{
EngineConstant: constants.OpenCodeEngine,
DefaultCommandName: "opencode",
ExtraCLIArgs: []string{"--print-logs", "--log-level", "DEBUG"},
MCPConfigFile: "opencode.jsonc",
StepName: "Execute OpenCode CLI",
ConfigStep: e.generateOpenCodeConfigStep(workflowData),
ModelEnvVarName: constants.OpenCodeCLIModelEnvVar,
WriteTimestamp: false,
})
}

// generateOpenCodeConfigStep writes opencode.jsonc with all permissions set to allow
Expand Down
8 changes: 3 additions & 5 deletions pkg/workflow/pi_engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,9 @@ func resolvePiBackend(workflowData *WorkflowData) UniversalLLMBackend {
}
// "github-copilot" is Pi CLI's internal name for GitHub Copilot. Accept it as
// an alias so workflows can use either "copilot/..." or "github-copilot/...".
parts := strings.SplitN(model, "/", 2)
if strings.EqualFold(parts[0], "github-copilot") {
return UniversalLLMBackendCopilot
}
backend, err := resolveUniversalLLMBackendFromModel(model)
backend, err := resolveBackendWithAliases(model, map[string]UniversalLLMBackend{
"github-copilot": UniversalLLMBackendCopilot,
})
if err != nil {
piLog.Printf("Could not resolve backend for Pi model %q, defaulting to copilot: %v", model, err)
return UniversalLLMBackendCopilot
Expand Down
Loading
Loading