Fix model env var to use provider-specific name#40
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR introduces provider-aware LLM model environment variable configuration. A new helper function ChangesProvider-aware LLM model environment variables
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @jameswnl. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
Bot
added
the
needs-ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/proposal/sandbox_templates.go`:
- Around line 308-317: The modelEnvVarForProvider switch incorrectly falls
through to default for Azure; update the function (modelEnvVarForProvider) to
add an explicit case for agenticv1alpha1.LLMProviderAzureOpenAI that returns the
correct env var (e.g., "OPENAI_MODEL") instead of letting it return
"ANTHROPIC_MODEL", and add/adjust a unit test (patchLLMCredentials_AzureOpenAI)
asserting that patchLLMCredentials produces the sandbox env var expected for
Azure (matching the AZURE_OPENAI_ENDPOINT mapping), mirroring the existing
Anthropic test pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7bdb95ac-91f6-49b0-a640-c400787e1bf4
📒 Files selected for processing (1)
controller/proposal/sandbox_templates.go
| func modelEnvVarForProvider(t agenticv1alpha1.LLMProviderType) string { | ||
| switch t { | ||
| case agenticv1alpha1.LLMProviderOpenAI: | ||
| return "OPENAI_MODEL" | ||
| case agenticv1alpha1.LLMProviderGoogleCloudVertex: | ||
| return "GEMINI_MODEL" | ||
| default: | ||
| return "ANTHROPIC_MODEL" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any references to model env var names to confirm expected sandbox contract.
rg -nP '\b(AZURE_OPENAI_MODEL|OPENAI_MODEL|GEMINI_MODEL|ANTHROPIC_MODEL)\b'
# Inspect tests covering modelEnvVarForProvider / patchLLMCredentials for each provider.
rg -nP -C3 'modelEnvVarForProvider|patchLLMCredentials' --type=goRepository: openshift/lightspeed-agentic-operator
Length of output: 5061
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find where AZURE_OPENAI_ENDPOINT is used/mapped
rg -n --hidden -S "AZURE_OPENAI_ENDPOINT" .
# 2) Find any other Azure-specific env var names (esp. *MODEL)
rg -n --hidden -S "AZURE.*MODEL|MODEL.*AZURE|AZURE_.*" controller/proposal/sandbox_templates.go controller/proposal/sandbox_templates_test.go || true
# 3) Check if tests cover AzureOpenAI in patchLLMCredentials / modelEnvVarForProvider
rg -n --hidden -S "AzureOpenAI|AZURE_OPENAI" controller/proposal/sandbox_templates_test.go
# 4) Show providerURLEnvVar implementation and nearby helpers for env var mapping
sed -n '280,380p' controller/proposal/sandbox_templates.goRepository: openshift/lightspeed-agentic-operator
Length of output: 4434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show AzureOpenAI-related tests (if any) and model/env var assertions
rg -n "AzureOpenAI|AZURE_OPENAI|OPENAI_MODEL|ANTHROPIC_MODEL|GEMINI_MODEL|AWSBedrock|CLAUDE_CODE_USE_BEDROCK" controller/proposal/sandbox_templates_test.go
# Print the relevant sections around patchLLMCredentials tests
sed -n '200,360p' controller/proposal/sandbox_templates_test.goRepository: openshift/lightspeed-agentic-operator
Length of output: 6590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused checks in the file under review
rg -n "AZURE_|OPENAI_|MODEL|ANTHROPIC_MODEL|OPENAI_MODEL|GEMINI_MODEL" controller/proposal/sandbox_templates.go
# Show the patchLLMCredentials + helper functions area with line context
sed -n '140,380p' controller/proposal/sandbox_templates.goRepository: openshift/lightspeed-agentic-operator
Length of output: 8213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "const Err|Err[A-Za-z0-9_]+" controller/proposal/sandbox_templates.goRepository: openshift/lightspeed-agentic-operator
Length of output: 3470
Fix AzureOpenAI model env var mapping (avoid setting ANTHROPIC_MODEL for Azure)
controller/proposal/sandbox_templates.go lines 308-317: modelEnvVarForProvider has cases for OpenAI and GoogleCloudVertex only; LLMProviderAzureOpenAI falls to default, so patchLLMCredentials sets ANTHROPIC_MODEL for Azure. This is inconsistent with the Azure-specific URL mapping (AZURE_OPENAI_ENDPOINT set in patchLLMCredentials), and will likely break the sandbox contract for Azure by reintroducing the “non-Anthropic gets ANTHROPIC_MODEL” issue.
🐛 Proposed fix
func modelEnvVarForProvider(t agenticv1alpha1.LLMProviderType) string {
switch t {
case agenticv1alpha1.LLMProviderOpenAI:
+ case agenticv1alpha1.LLMProviderOpenAI, agenticv1alpha1.LLMProviderAzureOpenAI:
return "OPENAI_MODEL"
case agenticv1alpha1.LLMProviderGoogleCloudVertex:
return "GEMINI_MODEL"
default:
return "ANTHROPIC_MODEL"
}
}Add/adjust a patchLLMCredentials_AzureOpenAI test asserting the model env var the sandbox expects for Azure (e.g., OPENAI_MODEL), similar to how the Anthropic test asserts ANTHROPIC_MODEL.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func modelEnvVarForProvider(t agenticv1alpha1.LLMProviderType) string { | |
| switch t { | |
| case agenticv1alpha1.LLMProviderOpenAI: | |
| return "OPENAI_MODEL" | |
| case agenticv1alpha1.LLMProviderGoogleCloudVertex: | |
| return "GEMINI_MODEL" | |
| default: | |
| return "ANTHROPIC_MODEL" | |
| } | |
| } | |
| func modelEnvVarForProvider(t agenticv1alpha1.LLMProviderType) string { | |
| switch t { | |
| case agenticv1alpha1.LLMProviderOpenAI, agenticv1alpha1.LLMProviderAzureOpenAI: | |
| return "OPENAI_MODEL" | |
| case agenticv1alpha1.LLMProviderGoogleCloudVertex: | |
| return "GEMINI_MODEL" | |
| default: | |
| return "ANTHROPIC_MODEL" | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/proposal/sandbox_templates.go` around lines 308 - 317, The
modelEnvVarForProvider switch incorrectly falls through to default for Azure;
update the function (modelEnvVarForProvider) to add an explicit case for
agenticv1alpha1.LLMProviderAzureOpenAI that returns the correct env var (e.g.,
"OPENAI_MODEL") instead of letting it return "ANTHROPIC_MODEL", and add/adjust a
unit test (patchLLMCredentials_AzureOpenAI) asserting that patchLLMCredentials
produces the sandbox env var expected for Azure (matching the
AZURE_OPENAI_ENDPOINT mapping), mirroring the existing Anthropic test pattern.
…ODEL The sandbox reads OPENAI_MODEL for the OpenAI provider and GEMINI_MODEL for Gemini, but the operator always set ANTHROPIC_MODEL regardless of provider type. This caused the sandbox to fall back to its default model (claude-opus-4-6) even when the Agent CR specified a different model, breaking non-Anthropic providers. Add modelEnvVarForProvider() that maps LLMProvider to the correct env var name. For Vertex, consults GoogleCloudVertex.ModelProvider to pick the right SDK env var (Anthropic on Vertex → ANTHROPIC_MODEL, Google models → GEMINI_MODEL, OpenAI-compat → OPENAI_MODEL). AzureOpenAI maps to OPENAI_MODEL since it uses the same SDK. Note: this is a stopgap until both operator and sandbox migrate to the generic LIGHTSPEED_MODEL env var per spec rule 16a (OLS-3153). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jameswnl
force-pushed
the
fix/provider-model-env-var
branch
from
fee7df6 to
aae895b
Compare
1 participant
Summary
OPENAI_MODELfor OpenAI andGEMINI_MODELfor Gemini providers, but the operator always setANTHROPIC_MODELregardless of provider typeclaude-opus-4-6) when using non-Anthropic providers, resulting in API errors (e.g., "model not found" when using OpenAI)modelEnvVarForProvider()helper that mapsLLMProviderTypeto the correct env var nameTest plan
OPENAI_MODEL=gpt-5.5correctly set in sandbox pod envANTHROPIC_MODELstill used (default case)go test ./controller/proposal/... -count=1passes🤖 Generated with Claude Code
Summary by CodeRabbit