Conversation
kikoso
left a comment
There was a problem hiding this comment.
The workflow half is the same change I approved in android-maps-compose #995 and is fine in principle. Two issues: the fallback does not fire when the repo variable is unset, and the GeminiVisualTestHelper half reads an env var that never reaches the app process on device.
One process note: the description says the PR only touches .github/workflows/update-skill.yml, but it also changes visual-testing/.../GeminiVisualTestHelper.kt, which is app code with a different failure mode. Could you add that to the summary? It is the part that actually needs review.
| - name: Update Skill via Gemini | ||
| env: | ||
| GEMINI_API_KEY: ${{ secrets.GEMINI_API_KEY }} | ||
| GEMINI_MODEL: ${{ vars.GEMINI_MODEL }} |
There was a problem hiding this comment.
When the GEMINI_MODEL repository variable is not set, ${{ vars.GEMINI_MODEL }} expands to the empty string, and Actions still defines the env var. .github/scripts/update_skill.py:21 uses os.getenv("GEMINI_MODEL", "gemini-3.8-flash"), whose default only applies when the name is absent, not when it is empty. So it gets "", and line 22 builds .../v1beta/models/:generateContent?key=....
Net effect: adding this line breaks update-skill on every repo that has not set the variable, where before it used a hardcoded model and worked.
The fix belongs in the script rather than here, os.getenv("GEMINI_MODEL") or "gemini-3.8-flash", but since this PR is what introduces the empty value, it should go in together.
| val fullPrompt = "$systemPrompt\n\nCommand: \"$prompt\"\n\nUI Hierarchy:\n$hierarchyXml" | ||
|
|
||
| val modelName = "gemini-3.5-flash" | ||
| val modelName = System.getenv("GEMINI_MODEL") ?: "gemini-3.8-flash" |
There was a problem hiding this comment.
This one will silently never do anything. GeminiVisualTestHelper is consumed from androidTest sources (maps3d-compose-demo, Maps3DSamples/ApiDemos/kotlin-app, Maps3DSamples/ComposeDemos/app all instantiate it in their BaseVisualTest), so it runs inside the app process on the device. That process does not inherit the Gradle or CI runner environment, so System.getenv("GEMINI_MODEL") is always null there and the elvis branch always wins. The model stays pinned to gemini-3.8-flash no matter what the repo variable says, which is the opposite of what the PR title promises.
The class already shows the right pattern: every public entry point (performActionFromPrompt, analyzeImage, analyzeImageBlocking) takes apiKey: String as a parameter rather than reading the environment, and BaseVisualTest sources it from BuildConfig.GEMINI_API_KEY via the secrets plugin, exactly because env vars do not survive the hop onto the device.
I would follow that: thread the model in the same way it is already done for the key, either as a defaulted parameter or a second BuildConfig field, so secrets.properties and CI both drive it through one mechanism.
suspend fun analyzeImage(
bitmap: Bitmap,
prompt: String,
apiKey: String,
model: String = BuildConfig.GEMINI_MODEL,
)If you prefer to keep the signatures untouched, InstrumentationRegistry.getArguments().getString("GEMINI_MODEL") also works and can be passed with -Pandroid.testInstrumentationRunnerArguments.GEMINI_MODEL=.... Either is fine. System.getenv is the one option that cannot work.
| } | ||
|
|
||
| val response: HttpResponse = client.post("https://generativelanguage.googleapis.com/v1beta/models/gemini-3.5-flash:generateContent?key=$apiKey") { | ||
| val model = System.getenv("GEMINI_MODEL") ?: "gemini-3.8-flash" |
There was a problem hiding this comment.
Same as line 85, and it also drifts from the neighbouring code: listAvailableModels on line 166 is called from analyzeImage on line 190 and still hits v1/models unversioned. Not asking you to fix that here, just noting that once the model becomes configurable, that debug listing call on every analyzeImage invocation is an extra round trip per screenshot. Might be worth dropping in a follow-up.
Summary
Passes the
GEMINI_MODELrepository variable into theUpdate Skill via Geministep environment in.github/workflows/update-skill.yml.Background
The
.github/scripts/update_skill.pyscript checksos.getenv("GEMINI_MODEL", "gemini-3.8-flash"). ExposingGEMINI_MODEL: ${{ vars.GEMINI_MODEL }}in the workflow environment allows repository-level configuration to take effect dynamically.Changes
.github/workflows/update-skill.ymlto includeGEMINI_MODEL: ${{ vars.GEMINI_MODEL }}in theUpdate Skill via Geministep environment.