Skip to content

ci: pass GEMINI_MODEL repository variable to update-skill workflow - #66

Open
dkhawk wants to merge 2 commits into
mainfrom
ci/update-skill-pass-gemini-model
Open

dkhawk wants to merge 2 commits into
mainfrom
ci/update-skill-pass-gemini-model

Conversation

@dkhawk

@dkhawk dkhawk commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

Passes the GEMINI_MODEL repository variable into the Update Skill via Gemini step environment in .github/workflows/update-skill.yml.

Background

The .github/scripts/update_skill.py script checks os.getenv("GEMINI_MODEL", "gemini-3.8-flash"). Exposing GEMINI_MODEL: ${{ vars.GEMINI_MODEL }} in the workflow environment allows repository-level configuration to take effect dynamically.

Changes

  • Updated .github/workflows/update-skill.yml to include GEMINI_MODEL: ${{ vars.GEMINI_MODEL }} in the Update Skill via Gemini step environment.

@dkhawk
dkhawk requested a review from LoyalAbbas September 9, 2026 19:20
@dkhawk
dkhawk marked this pull request as ready for review September 14, 2026 21:41
@dkhawk
dkhawk requested a review from kikoso September 14, 2026 21:41

@kikoso kikoso left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants