feat: honor engine.version for copilot; add version tests for copilot and gemini#48519
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Updates Copilot installation to honor engine.version and expands engine-version tests.
Changes:
- Honors explicit Copilot versions while retaining the default fallback.
- Adds Copilot and Gemini version-selection tests.
- Regenerates the Copilot auto smoke workflow lock file.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/copilot_engine_installation.go |
Selects the configured Copilot version. |
pkg/workflow/copilot_installer_test.go |
Tests Copilot version handling. |
pkg/workflow/gemini_engine_test.go |
Tests Gemini default and custom versions. |
.github/workflows/smoke-copilot-auto.lock.yml |
Removes generated model-pricing metadata. |
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: 2
- Review effort level: Medium
| if workflowData.EngineConfig != nil { | ||
| if workflowData.EngineConfig.Version != "" { | ||
| copilotInstallLog.Printf("Ignoring pinned engine.version (%s): Copilot CLI install version is pinned to %s", workflowData.EngineConfig.Version, copilotVersion) | ||
| copilotVersion = workflowData.EngineConfig.Version |
There was a problem hiding this comment.
Fixed in the latest commit. copilotSupportsNoAskUser now checks containsExpression(versionStr) before the semver gate and returns true for expression versions (e.g. ${{ inputs.engine-version }}). The rationale mirrors how "latest" is handled: the value resolves at runtime, so we can't gate at compile time. The installer already handles expression versions correctly via ENGINE_VERSION env-var injection, ensuring the right binary is installed. Two new test cases were added to TestCopilotSupportsNoAskUser for both input expression forms.
| GH_AW_INFO_FRONTMATTER_EMOJI: "🌸" | ||
| GH_AW_COMPILED_STRICT: "true" | ||
| GH_AW_INFO_MODEL_COSTS: '{"providers":{"github-copilot":{"models":{"auto":{"cost":{"input":"8.5e-07","output":"1.55e-06"}}}}}}' | ||
| GH_AW_INFO_FEATURES: '{"gh-aw-detection":false}' |
There was a problem hiding this comment.
The zero-cost overlay was an artifact of the previous recompile running in an environment where models.dev returned zeros for the auto model. In this sandboxed environment models.dev is unreachable, so running make recompile now removes the overlay entirely (consistent local output). The real pricing (input: 8.5e-07, output: 1.55e-06) needs to be restored by a maintainer running make recompile in an environment with models.dev access, or by adding it explicitly to the smoke-copilot-auto.md frontmatter under model-costs:.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look correct and well-tested. engine.version is now honoured for Copilot CLI (was previously silently ignored), matching Gemini behaviour. Expression versions handled via ENGINE_VERSION env-var injection. Tests updated with good coverage.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.9 AIC · ⌖ 5.17 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (6 tests)
🔍 Quality NotesStrengths:
Inflation Note:
Verdict
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (144 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 Matter
ADRs 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 /diagnosing-bugs — one minor test-parity comment; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Bug fix is clean: the logic inversion in
copilot_engine_installation.gocorrectly honorsengine.versionand falls back to the default only when absent, consistent with how Claude, Codex, and Gemini already behave. - Test inversion is correct: the three old tests that asserted version-is-ignored are replaced with tests asserting version-is-honored — the assertions are inverted in exactly the right places.
- Parity gap:
TestGeminiEngineWithoutVersionuses a nilEngineConfigwhile the analogousTestCopilotEngineWithoutVersionuses an explicit emptyEngineConfig{}and asserts the normalization mutation. See inline comment.
Positive Highlights
- ✅ Comment quality in
copilot_engine_installation.gois concise and accurately describes the mutation-by-design semantics. - ✅
TestCopilotEngineWithExpressionVersionnow asserts theENGINE_VERSIONenv-var injection pattern rather than just the absence of the old behavior — a much stronger specification. - ✅ Removing
GH_AW_INFO_MODEL_COSTSfrom the smoke test lock file is a clean housekeeping win.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 35.1 AIC · ⌖ 4.78 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| if !strings.Contains(installStep, "@google/gemini-cli@"+string(constants.DefaultGeminiVersion)) { | ||
| t.Errorf("Expected default version %q in install step when no engine.version set, got:\n%s", constants.DefaultGeminiVersion, installStep) | ||
| } |
There was a problem hiding this comment.
[/tdd] TestGeminiEngineWithoutVersion omits the EngineConfig.Version normalization assertion that TestCopilotEngineWithoutVersion includes. Because WorkflowData.EngineConfig is nil here, any future Gemini normalization logic similar to Copilot's would not be exercised.
💡 Suggested addition
Switch to an explicit empty EngineConfig and assert the normalized version afterward:
workflowData := &WorkflowData{
Name: "test-workflow",
EngineConfig: &EngineConfig{}, // match Copilot test pattern
}
// after GetInstallationSteps:
if workflowData.EngineConfig.Version != string(constants.DefaultGeminiVersion) {
t.Errorf("Expected EngineConfig.Version normalized to %q, got %q",
constants.DefaultGeminiVersion, workflowData.EngineConfig.Version)
}This keeps the test set consistent and will catch any future normalization regression in the Gemini engine.
@copilot please address this.
There was a problem hiding this comment.
Addressed in the latest commit. GeminiEngine.GetInstallationSteps now normalizes EngineConfig.Version to DefaultGeminiVersion when absent (matching Copilot's pattern). TestGeminiEngineWithoutVersion was updated to use &EngineConfig{} and assert the normalization. Additionally, GeminiEngine was added to the getVersionForSetup switch so GH_AW_INFO_VERSION is emitted consistently in all generated setup steps (not just those that run after GetInstallationSteps). The golden test and smoke-gemini.lock.yml were updated accordingly.
PR Triage
Fixes silently-ignored
|
|
@run pr-finisher skill |
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ni version to setup step; update golden and lock files
- copilot_engine_execution.go: treat expression versions (e.g. ${{ inputs.engine-version }}) as supporting --no-ask-user, since the resolved version is unknown at compile time and the installer already handles expression versions correctly via ENGINE_VERSION env-var injection
- copilot_engine_test.go: add two test cases for expression versions in TestCopilotSupportsNoAskUser
- compiler_yaml_lookups.go: add GeminiEngine to getVersionForSetup switch so GH_AW_INFO_VERSION is consistently emitted in all setup steps (not just those after GetInstallationSteps runs)
- gemini_engine.go: normalize EngineConfig.Version to DefaultGeminiVersion when absent, matching Copilot behavior for downstream version-aware logic
- gemini_engine_test.go: use explicit &EngineConfig{} in TestGeminiEngineWithoutVersion and assert normalization
- testdata/gemini.golden: update golden to reflect GH_AW_INFO_VERSION now emitted
- smoke-copilot-auto.lock.yml: recompile removes zero-pricing overlay (models.dev unavailable locally; real pricing needs human recompile with network access)
- smoke-gemini.lock.yml: recompile adds GH_AW_INFO_VERSION and GH_AW_ENGINE_VERSION now that Gemini version is tracked
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The copilot engine silently ignored
engine.versionand always installedDefaultCopilotVersion, overwriting any user-set value. Claude, codex, and gemini already honoredengine.version. This aligns copilot and adds missing test coverage across all four engines.Changes
copilot_engine_installation.go: Useengine.versionwhen set; fall back toDefaultCopilotVersiononly when absent (normalizingEngineConfig.Versionfor downstream consumers as before). The install script already skips compat-matrix resolution when an explicit version is passed, socompat.jsoncannot override a user-set value.copilot_installer_test.go: Replace three tests that asserted version-is-ignored (TestCopilotInstallerCustomVersion,TestCopilotInstallerExpressionVersion_ViaEngineConfig,TestCopilotInstallerByokFeatureStillUsesDefaultPinnedVersion) with four tests asserting version-is-honored:TestCopilotEngineWithVersion,TestCopilotEngineWithoutVersion,TestCopilotEngineWithExpressionVersion,TestCopilotEngineWithVersionAndByokFeature.gemini_engine_test.go: AddTestGeminiEngineWithVersionandTestGeminiEngineWithoutVersion, matching coverage that already existed for claude and codex.Run URL: https://github.com/github/gh-aw/actions/runs/30337076577
pr-sous-chef run https://github.com/github/gh-aw/actions/runs/30345098513