refactor: extract shared CLI engine execution logic; make Pi backend resolution configurable#41921
Conversation
…backend aliases - Add resolveBackendWithAliases() to universal_llm_consumer_engine.go, allowing engines to declare custom provider name aliases before falling back to the standard copilot/anthropic/openai/codex resolution. - Add UniversalCLIEngineExecutionConfig struct and BuildCLIEngineExecutionSteps() method on UniversalLLMConsumerEngine to capture the common execution step pattern shared by Crush and OpenCode: config-file step, 'run' subcommand, firewall-aware domain resolution, standard AWF env vars, and step formatting. - Simplify CrushEngine.GetExecutionSteps and OpenCodeEngine.GetExecutionSteps to delegate to BuildCLIEngineExecutionSteps, removing ~130 lines of duplicated code between the two engines. - Update resolvePiBackend in pi_engine.go to use resolveBackendWithAliases with the 'github-copilot' -> copilot alias, making the alias handling explicit and reusable by other engines that may expose custom provider names. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #41921 changes only production Go files: pkg/workflow/crush_engine.go, opencode_engine.go, pi_engine.go, universal_llm_consumer_engine.go. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This PR reduces duplication across the “universal LLM consumer” CLI engines by extracting the shared GitHub Actions execution-step construction into UniversalLLMConsumerEngine, and it makes Pi’s backend resolution extensible by introducing an alias-aware backend resolver.
Changes:
- Added
resolveBackendWithAliases()to support provider-prefix aliases (e.g. Pi’sgithub-copilot/...) while reusing the standard universal backend resolution. - Introduced
UniversalCLIEngineExecutionConfig+BuildCLIEngineExecutionSteps()to centralize the shared “run subcommand + AWF wrapping + env injection” step generation. - Updated Crush and OpenCode engines to use the shared execution-step builder; updated Pi to use alias-based backend resolution.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/universal_llm_consumer_engine.go | Adds alias-aware backend resolution and the shared CLI execution step builder/config used by multiple engines. |
| pkg/workflow/pi_engine.go | Switches Pi’s backend resolution to use the new alias-aware resolver (for github-copilot/...). |
| pkg/workflow/opencode_engine.go | Refactors OpenCode execution step generation to delegate to BuildCLIEngineExecutionSteps(). |
| pkg/workflow/crush_engine.go | Refactors Crush execution step generation to delegate to BuildCLIEngineExecutionSteps(). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Low
…extraction Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (208 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /zoom-out, and /improve-codebase-architecture — commenting with improvement suggestions; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Missing unit tests for the new helpers:
resolveBackendWithAliasesis a new reusable function with edge-case logic (case-insensitive matching, TrimSpace, slash-requirement) but has no direct tests. TheBuildCLIEngineExecutionStepspath is well-covered indirectly by the existingGetExecutionStepstests incrush_engine_test.goandopencode_engine_test.go. - Implicit API contracts on
resolveBackendWithAliases: Alias keys must be pre-lowercased and alias matching only applies to"provider/model"strings. Neither constraint is documented or enforced, creating a subtle trap for future callers. - Silent skip of
ConfigStep: The empty-check guard makes a missing config step look intentional; a brief comment or rename would clarify intent for the next engine author.
Positive Highlights
- ✅ Excellent PR description — the call-site diff is shown in the body, making the extraction easy to verify
- ✅ Clean struct design:
UniversalCLIEngineExecutionConfigis well-doc-commented field by field - ✅ Correct behavioral equivalence across Crush, OpenCode, and Pi — verified field by field against the original logic
- ✅
resolveBackendWithAliasescorrectly integrates with pi_engine.go's existing pre-slash guard at line 76 - ✅
cliArgs := append([]string{}, cfg.ExtraCLIArgs...)is a good defensive copy
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 82.1 AIC · ⌖ 10.1 AIC · ⊞ 6.6K
| // lowercased provider names (the prefix before the first "/") to the corresponding | ||
| // UniversalLLMBackend. This lets engines that expose their own provider naming (e.g. Pi's | ||
| // "github-copilot") reuse the shared backend resolution logic without special-casing. | ||
| func resolveBackendWithAliases(model string, extraAliases map[string]UniversalLLMBackend) (UniversalLLMBackend, error) { |
There was a problem hiding this comment.
[/tdd] resolveBackendWithAliases has no unit tests — add coverage for at least: (a) exact alias match, (b) case-insensitive match ("GITHUB-COPILOT/model" → UniversalLLMBackendCopilot), (c) unknown provider falls through to resolveUniversalLLMBackendFromModel, and (d) empty extraAliases bypasses alias check.
💡 Suggested test skeleton
func TestResolveBackendWithAliases(t *testing.T) {
aliases := map[string]UniversalLLMBackend{
"github-copilot": UniversalLLMBackendCopilot,
}
tests := []struct {
model string
want UniversalLLMBackend
wantErr bool
}{
{"github-copilot/gpt-4", UniversalLLMBackendCopilot, false},
{"GITHUB-COPILOT/gpt-4", UniversalLLMBackendCopilot, false},
{"copilot/gpt-4", UniversalLLMBackendCopilot, false},
{"unknown/model", "", true},
}
for _, tc := range tests {
got, err := resolveBackendWithAliases(tc.model, aliases)
if tc.wantErr != (err != nil) {
t.Errorf("model=%q: wantErr=%v got err=%v", tc.model, tc.wantErr, err)
}
if !tc.wantErr && got != tc.want {
t.Errorf("model=%q: want %q got %q", tc.model, tc.want, got)
}
}
}@copilot please address this.
| if len(extraAliases) > 0 { | ||
| model = strings.TrimSpace(model) | ||
| parts := strings.SplitN(model, "/", 2) | ||
| if len(parts) == 2 { |
There was a problem hiding this comment.
[/zoom-out] The len(parts) == 2 guard means aliases only fire when the model string contains a /. Bare provider names like "github-copilot" (no slash) silently skip alias matching and fall through to resolveUniversalLLMBackendFromModel, which returns an error.
This is safe today because pi_engine.go has a pre-check (if !strings.Contains(model, "/")) before calling this function. But the function's docstring doesn't document this constraint, making it a hidden trap for future engines that call resolveBackendWithAliases directly.
💡 Suggestion
Add a sentence to the docstring:
// Alias matching only applies to "provider/model" strings (those containing a
// slash). Bare provider names without a slash are passed directly to
// resolveUniversalLLMBackendFromModel.
@copilot please address this.
| parts := strings.SplitN(model, "/", 2) | ||
| if len(parts) == 2 { | ||
| provider := strings.ToLower(strings.TrimSpace(parts[0])) | ||
| if backend, ok := extraAliases[provider]; ok { |
There was a problem hiding this comment.
[/improve-codebase-architecture] The extraAliases map keys are compared against a lowercased provider string (line 188 calls strings.ToLower), but there's no enforcement that callers supply lowercased keys. A future caller passing {"GitHub-Copilot": ...} would silently fail to match.
💡 Suggestion
Normalise keys on the way in to make the function self-defending:
for k, v := range extraAliases {
normalised[strings.ToLower(k)] = v
}Or add a // Keys must be lowercase note to the docstring so the contract is explicit and reviewable.
@copilot please address this.
| var steps []GitHubActionStep | ||
|
|
||
| // Prepend the config step (writes permissions JSON to workspace). | ||
| if len(cfg.ConfigStep) > 0 { |
There was a problem hiding this comment.
[/improve-codebase-architecture] ConfigStep is silently skipped when empty — if a future engine adopts BuildCLIEngineExecutionSteps and forgets to populate ConfigStep, the permissions config write is quietly omitted, which could cause the agent to hang on interactive prompts in CI.
The original engines always wrote a config step; this guard makes the omission look intentional rather than accidental.
💡 Suggestion
Consider renaming to PreSteps []GitHubActionStep and documenting that an empty slice is valid and means "no pre-step". Or add a comment next to the current guard:
// ConfigStep is optional; engines that need a permissions config file
// (e.g. .crush.json) should always populate this.
if len(cfg.ConfigStep) > 0 {
steps = append(steps, cfg.ConfigStep)
}Either approach surfaces the intent to the next engineer adding an engine.
@copilot please address this.
There was a problem hiding this comment.
Clean deduplication overall — the BuildCLIEngineExecutionSteps extraction is faithful and the resolveBackendWithAliases change to resolvePiBackend is behaviorally equivalent to the old inline EqualFold check.
### Findings summary
Medium — nil pointer in new shared entry point (universal_llm_consumer_engine.go:241)
workflowData.Name is dereferenced before any nil guard in BuildCLIEngineExecutionSteps. The old per-engine code had the same gap, but this method is now the single code path for all CLI engines so the blast radius is higher. See inline comment.
Medium — polymorphism bypass on secret name filter (universal_llm_consumer_engine.go:354)
BuildCLIEngineExecutionSteps calls GetUniversalRequiredSecretNames directly on the embedded base type, bypassing any GetRequiredSecretNames override a concrete engine might add. Harmless today (both Crush and OpenCode just delegate), but a silent correctness trap for future engines that extend the secret list. See inline comment.
Low — double space when ExtraCLIArgs is empty (universal_llm_consumer_engine.go:259)
Latent defect for future engines that pass no extra args. See inline comment.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 173 AIC · ⌖ 7.07 AIC · ⊞ 5.2K
| cfg UniversalCLIEngineExecutionConfig, | ||
| ) []GitHubActionStep { | ||
| universalLLMConsumerLog.Printf("Generating execution steps for %s engine: workflow=%s, firewall=%v", | ||
| cfg.DefaultCommandName, workflowData.Name, isFirewallEnabled(workflowData)) |
There was a problem hiding this comment.
Nil dereference if workflowData is nil: workflowData.Name is read unconditionally before any nil guard; a nil argument will panic here.
💡 Suggested fix
Add a nil guard at the top of BuildCLIEngineExecutionSteps, consistent with how the rest of the file handles nil *WorkflowData:
if workflowData == nil {
universalLLMConsumerLog.Printf("BuildCLIEngineExecutionSteps: nil workflowData for %s engine", cfg.DefaultCommandName)
return nil
}The old per-engine code had the same blind spot, but BuildCLIEngineExecutionSteps is now the single shared code path for every CLI engine that adopts it — so the blast radius is larger and a nil guard here protects all future callers.
| " - name: " + cfg.StepName, | ||
| " id: agentic_execution", | ||
| } | ||
| allowedSecrets := e.GetUniversalRequiredSecretNames(workflowData) |
There was a problem hiding this comment.
GetUniversalRequiredSecretNames bypasses polymorphism: the method is called directly on the embedded base struct, so any override of GetRequiredSecretNames in a concrete engine will not be used for the secret filter.
💡 Detail and suggested fix
In Go, BuildCLIEngineExecutionSteps has receiver *UniversalLLMConsumerEngine, so e is always the embedded base struct — not the outer *CrushEngine or *OpenCodeEngine. Calling e.GetUniversalRequiredSecretNames(workflowData) here is therefore statically bound to the base implementation.
Today this is harmless because CrushEngine.GetRequiredSecretNames and OpenCodeEngine.GetRequiredSecretNames both just delegate to GetUniversalRequiredSecretNames. But any future engine that calls BuildCLIEngineExecutionSteps and also adds extra secrets in its GetRequiredSecretNames override will silently have those extra secrets excluded from FilterEnvForSecrets, meaning they won't appear in the generated step's env: block.
The cleanest fix is to accept a secretNamesFn parameter:
func (e *UniversalLLMConsumerEngine) BuildCLIEngineExecutionSteps(
workflowData *WorkflowData,
logFile string,
cfg UniversalCLIEngineExecutionConfig,
secretNamesFn func(*WorkflowData) []string, // caller supplies e.GetRequiredSecretNames
) []GitHubActionStepAlternatively, expose it as a config field on UniversalCLIEngineExecutionConfig. Either way the caller controls which secret list gates the env filter.
| if workflowData.EngineConfig != nil && workflowData.EngineConfig.Command != "" { | ||
| commandName = workflowData.EngineConfig.Command | ||
| } | ||
| engineCommand := fmt.Sprintf("%s run %s %s", commandName, shellJoinArgs(cliArgs), promptArg) |
There was a problem hiding this comment.
Double space in engineCommand when ExtraCLIArgs is empty: shellJoinArgs(nil) returns "", so the format string produces "<binary> run \"$(cat ...)\"" with two consecutive spaces.
💡 Suggested fix
Both current callers (Crush: --verbose; OpenCode: --print-logs --log-level DEBUG) pass non-empty args, so there is no immediate breakage. But any future engine with no extra CLI args will silently get a double-space command. Shells handle it fine, but it's a latent inconsistency in generated YAML.
Fix by trimming or only including the args segment when non-empty:
var engineCommand string
if len(cliArgs) > 0 {
engineCommand = fmt.Sprintf("%s run %s %s", commandName, shellJoinArgs(cliArgs), promptArg)
} else {
engineCommand = fmt.Sprintf("%s run %s", commandName, promptArg)
}
|
@copilot please run the
|
|
🎉 This pull request is included in a new release. Release: |
Crush and OpenCode had ~90 lines of nearly-identical
GetExecutionStepslogic. Pi'sgithub-copilotprovider alias was inlined with a manualstrings.EqualFoldcheck, not reusable.Changes
universal_llm_consumer_engine.goresolveBackendWithAliases(model, extraAliases)— extends the standardcopilot/anthropic/openai/codexlookup with caller-supplied provider name aliases, checked first. Makes the Pigithub-copilotalias pattern available to any future engine without special-casing.UniversalCLIEngineExecutionConfig— config struct for the sharedrun-subcommand execution pattern: engine constant, command name, extra CLI flags, permissions config file, step name, model env var, timestamp behavior.BuildCLIEngineExecutionSteps()— method onUniversalLLMConsumerEngineimplementing the common execution step: config-writing step → firewall-aware AWF command → standard env injection (prompt, workspace, MCP config, safe-output, trace context, max-turns, model env) → step formatting.crush_engine.go/opencode_engine.goGetExecutionStepsin each engine collapses from ~90 lines to a singleBuildCLIEngineExecutionStepscall:pi_engine.goresolvePiBackendnow callsresolveBackendWithAliaseswith{"github-copilot": UniversalLLMBackendCopilot}instead of the inlineEqualFoldcheck.