Inject GH_AW_ENGINE_VERSION into custom engine execution steps - #50597
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Injects configured engine versions into behavior-defined engine execution environments.
Changes:
- Adds
applyEngineVersionEnv. - Applies it during custom engine execution.
- Covers literal, expression, absent, and nil configurations.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/engine_helpers.go |
Adds version environment helper. |
pkg/workflow/engine_helpers_test.go |
Tests helper behavior. |
pkg/workflow/behavior_defined_engine.go |
Injects version into execution environments. |
pkg/workflow/behavior_defined_engine_harness_test.go |
Tests rendered execution steps. |
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: Balanced
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
@copilot update goose.md, crush.md, opencode.md, aider.com to declare their version in the front matter and use the version variable in the generate code |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (2 tests)
Verdict
|
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
The implementation is correct and well-tested. applyEngineVersionEnv is properly nil-guarded, follows the existing helper pattern, and is correctly scoped to behavior-defined (custom) engines only. Test coverage handles nil, empty, literal, and expression cases.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 12.6 AIC · ⌖ 9.25 AIC · ⊞ 5.4K
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (120 new lines in Draft ADR committed:
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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — changes are solid, approving.
📋 Key Themes & Highlights
Positive Highlights
- ✅
applyEngineVersionEnvfollows the same nil-guard and early-return pattern asapplyEngineCwdEnv— consistent with existing conventions - ✅ Unit tests cover all five cases: nil data, nil config, empty version, literal version, and GHA expression
- ✅ Integration harness tests confirm both injection and absence end-to-end
- ✅ Raw expression (
${{ inputs.engine-version }}) is passed through unchanged — correct, since GHA evaluates it at runtime - ✅ Call-site ordering in
buildBehaviorDefinedExecutionEnvis logical alongsideapplyEngineCwdEnv
There was a problem hiding this comment.
Verdict: COMMENT — no blocking issues
Small, well-tested addition that injects GH_AW_ENGINE_VERSION following the exact same pattern as the existing applyEngineCwdEnv/GH_AW_ENGINE_CWD helper (nil-safe checks, same YAML env rendering path via appendEnvVarLine, which already handles escaping/newlines). Coverage includes literal version, GHA-expression version, and absent-version cases at both the helper unit-test level and the execution-step integration level.
Notes considered
- No new injection surface: reuses
FormatStepWithCommandAndEnv→appendEnvVarLine, the same trusted path used forCwd. - No validation of
Versionformat, but this matches existingCwd/MaxTurnshandling — consistent with current conventions, not a regression. - Docs (PR body) accurately describe scope; audit-log injection (
GH_AW_INFO_VERSION) was already covered and correctly left untouched.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in 24883d0. The shared engine frontmatter now declares versions; Goose and Aider consume |
|
@copilot please refresh this branch, address any remaining review feedback or failed checks, and run the
|
| python: | ||
| version: "3.12" | ||
| env: | ||
| GH_AW_ENGINE_VERSION: "0.86.2" |
There was a problem hiding this comment.
@copilot not needed since it is populated by the compiller
There was a problem hiding this comment.
Addressed in 1bb6e43. The compiler now sets GH_AW_ENGINE_VERSION at agent-job scope, so the Aider pre-agent install receives it without an imported top-level env declaration.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
engine.versionwas parsed and used for installation but never injected into the main agent execution environment for behavior-defined engines (Goose, Ader, Crush, etc.), making it inaccessible to scripts at runtime. Version was already in safe-outputs jobs and audit info (GH_AW_INFO_VERSION).Changes
engine_helpers.go— NewapplyEngineVersionEnvhelper setsGH_AW_ENGINE_VERSIONfromEngineConfig.Version; supports literal strings and GHA expressions (${{ inputs.engine-version }})behavior_defined_engine.go— CallsapplyEngineVersionEnvinbuildBehaviorDefinedExecutionEnv, alongside existingapplyEngineCwdEnvGH_AW_ENGINE_VERSIONAudit logging (
GH_AW_INFO_VERSION/GH_AW_INFO_AGENT_VERSION) already covered custom engines viagetInstallationVersion— no changes needed there.