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 |
| 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}' |
![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.
PR Triage
Fixes silently-ignored
|
|
@run pr-finisher skill |
|
@copilot run pr-finisher skill |
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