Route model to engine via native CLI environment variables#17296
Route model to engine via native CLI environment variables#17296
Conversation
…ed (#issue) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot you need to use the env var support by copilot-cli |
…opilot engine Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in 844f237. When Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot extend the agentic engine interface to provide the name of the model environment variable. In that case, the compiler should set the variable to the model id. Research and apply to Anthropic Claude, OpenAI Codex, GitHub Copilot CLI |
…vars for Copilot, Claude, and Gemini Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 2ab9ffc. Added
For engines with a native env var, model is now always set as an env var (both static values and GitHub Actions expressions), removing the need to embed Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🧪 Smoke Temporary ID is now testing temporary ID functionality... |
|
🧪 Smoke Project is now testing project operations... |
|
✅ Smoke Project completed successfully. All project operations validated. |
|
Add @copilot, merge |
|
Smoke test results for
Overall: Run: https://github.com/github/gh-aw/actions/runs/22244922259
|
There was a problem hiding this comment.
Reviewed PR #17296 "Route model to engine via native CLI environment variables". The approach of using native CLI environment variables (COPILOT_MODEL, ANTHROPIC_MODEL, GEMINI_MODEL) is clean and avoids template injection issues. The ModelEnvVarProvider interface is well-structured with good defaults via BaseEngine. Build passes ✅.
📰 BREAKING: Report filed by Smoke Copilot
|
|
||
| // ClaudeCLIModelEnvVar is the native environment variable name supported by the Claude Code CLI | ||
| // for selecting the model. Setting this env var is equivalent to passing --model to the CLI. | ||
| ClaudeCLIModelEnvVar = "ANTHROPIC_MODEL" |
There was a problem hiding this comment.
Good use of named constants for the native CLI env vars. This makes it easy to update them in one place if the CLIs change their env var names in the future.
| // GetModelEnvVarName returns the name of the native environment variable the CLI | ||
| // uses for model selection (e.g., "COPILOT_MODEL", "ANTHROPIC_MODEL", "GEMINI_MODEL"). | ||
| // Returns an empty string if the engine does not support a native model env var. | ||
| GetModelEnvVarName() string |
There was a problem hiding this comment.
The ModelEnvVarProvider interface is clean and well-documented. One suggestion: consider adding a note in the interface doc that returning "" means the engine relies on the existing GH_AW_MODEL_* environment variable fallback, to clarify the contract for future implementors.
There was a problem hiding this comment.
Pull request overview
Routes engine model selection through environment variables to avoid embedding GitHub Actions expressions in shell commands (preventing template injection validation failures) and to leverage engines’ native model env var support.
Changes:
- Added
ModelEnvVarProviderto theCodingAgentEnginecomposite interface, with a default no-op implementation onBaseEngine. - Updated Copilot/Claude/Gemini engines to set native model env vars (
COPILOT_MODEL,ANTHROPIC_MODEL,GEMINI_MODEL) whenengine.modelis configured; Codex continues using GH_AW_* env vars + shell expansion. - Updated/added tests to assert model is provided via env vars and not embedded as
--model ...in commands for engines that support native env vars.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/agentic_engine.go | Adds ModelEnvVarProvider interface and composes it into CodingAgentEngine; provides BaseEngine default. |
| pkg/constants/constants.go | Introduces constants for native CLI model env var names. |
| pkg/workflow/copilot_engine.go | Implements GetModelEnvVarName() for Copilot. |
| pkg/workflow/copilot_engine_execution.go | Switches Copilot model handling to native env var when configured; keeps GH_AW_* fallback + shell expansion when not configured. |
| pkg/workflow/claude_engine.go | Switches Claude model handling to native env var when configured; keeps GH_AW_* fallback + shell expansion when not configured. |
| pkg/workflow/codex_engine.go | Routes configured model through GH_AW_* env vars consistently, always using shell expansion for -c model=.... |
| pkg/workflow/gemini_engine.go | Switches configured model to GEMINI_MODEL; retains GH_AW_* env var fallback (but currently does not apply it to the command). |
| pkg/workflow/threat_detection_test.go | Updates detection test expectations to look for env var model wiring (not --model). |
| pkg/workflow/model_env_vars_test.go | Adds coverage for explicit/expression models using env vars and for GetModelEnvVarName(). |
| pkg/workflow/gemini_engine_test.go | Updates model test to assert GEMINI_MODEL is set and --model is not embedded. |
| pkg/workflow/engine_config_test.go | Updates expectations around model propagation (env var vs CLI flag). |
| pkg/workflow/claude_engine_test.go | Updates model assertions to env var wiring. |
| pkg/workflow/claude_engine_network_test.go | Updates network-permissions tests to check env var model wiring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Model is passed via the native GEMINI_MODEL env var (not as a --model flag) | ||
| assert.Contains(t, stepContent, "GEMINI_MODEL: gemini-1.5-pro", "Should set GEMINI_MODEL env var") | ||
| assert.NotContains(t, stepContent, "--model gemini-1.5-pro", "Should not embed model in command") |
There was a problem hiding this comment.
The tests verify that GH_AW_MODEL_AGENT_GEMINI / GH_AW_MODEL_DETECTION_GEMINI appear in the step YAML, but they don’t assert that those env vars actually affect the executed command. Given the current implementation, it’s possible for these vars to be present yet unused.
Add an assertion for the unconfigured-model case that the step either sets GEMINI_MODEL from the GH_AW vars or conditionally adds a --model argument based on GH_AW_MODEL_*_GEMINI (matching the intended behavior described in the PR).
| // Model is always passed via the native GEMINI_MODEL environment variable when configured. | ||
| // This avoids embedding the value directly in the shell command (which fails template injection | ||
| // validation for GitHub Actions expressions like ${{ inputs.model }}). | ||
| // Fallback for unconfigured model uses GH_AW_MODEL_AGENT_GEMINI with shell expansion. | ||
| modelConfigured := workflowData.EngineConfig != nil && workflowData.EngineConfig.Model != "" | ||
|
|
There was a problem hiding this comment.
In the unconfigured-model path, Gemini sets GH_AW_MODEL_AGENT_GEMINI / GH_AW_MODEL_DETECTION_GEMINI in env, but the generated geminiCommand never references those variables (no conditional --model shell expansion, and GEMINI_MODEL is only set when modelConfigured). As a result, setting GH_AW_MODEL_*_GEMINI will not actually change the model used by the Gemini CLI.
Update the command/env logic so the fallback env var influences execution (e.g., either set GEMINI_MODEL from the GH_AW vars when not explicitly configured, or append a conditional --model "$GH_AW_MODEL_*_GEMINI" segment to geminiCommand).
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Smoke test results:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 2a2313c. Merged main and recompiled all 153 workflow files. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
ModelEnvVarProviderinterface toagentic_engine.gowithGetModelEnvVarName() stringGetModelEnvVarName()returning""toBaseEngineModelEnvVarProvidertoCodingAgentEnginecomposite interfaceClaudeCLIModelEnvVar = "ANTHROPIC_MODEL"andGeminiCLIModelEnvVar = "GEMINI_MODEL"constantsGetModelEnvVarName()onCopilotEngine→COPILOT_MODELGetModelEnvVarName()onClaudeEngine→ANTHROPIC_MODELGetModelEnvVarName()onCodexEngine→""(no native env var)GetModelEnvVarName()onGeminiEngine→GEMINI_MODELCOPILOT_MODELenv var; no--modelflagANTHROPIC_MODELenv var; no--modelflagGEMINI_MODELenv var; no--modelflagGH_AW_MODEL_*_CODEXwith shell expansionOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.