feat: dynamic model support via MODELS_JSON ConfigMap #612
Conversation
Models were hardcoded in the frontend dropdown and runner's Vertex mapping, requiring code changes + image rebuilds to add a model. Now models are defined in the existing operator-config ConfigMap and flow through the stack: backend exposes them via /api/cluster-info, frontend populates the dropdown dynamically, operator passes them to runner pods, and the runner builds VERTEX_MODEL_MAP from the env var. Edit ConfigMap + rollout restart = models updated, no image rebuilds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Claude Code ReviewSummaryThis PR introduces dynamic model configuration via a Overall Assessment: Strong implementation with good fallback patterns and type safety. A few areas need attention before merge. Issues by Severity🔴 Critical Issues1. Backend: Untyped JSON parsing exposes type safety risks
type ModelInfo struct {
Name string `json:"name"`
DisplayName string `json:"displayName"`
VertexID string `json:"vertexId,omitempty"`
Default bool `json:"default,omitempty"`
}
var models []ModelInfo
if raw := os.Getenv("MODELS_JSON"); raw \!= "" {
if err := json.Unmarshal([]byte(raw), &models); err \!= nil {
log.Printf("Warning: failed to parse MODELS_JSON: %v", err)
models = []ModelInfo{}
}
}
// Validate required fields
for _, m := range models {
if m.Name == "" || m.DisplayName == "" {
log.Printf("Warning: invalid model entry (missing name or displayName)")
}
}2. Frontend: Defaulting model value may cause form inconsistency
useEffect(() => {
if (clusterModels.length > 0) {
const defaultModel = clusterModels.find((m) => m.default)?.name ?? "claude-sonnet-4-5";
form.setValue("model", defaultModel);
}
}, [clusterModels, form]);🟡 Major Issues3. Python: Broad exception catching masks errors
try:
models = _json.loads(raw)
return {m["name"]: m["vertexId"] for m in models if m.get("vertexId")}
except (_json.JSONDecodeError, KeyError, TypeError) as e:
logger.warning(f"Failed to parse MODELS_JSON: {e}")
return dict(_HARDCODED_VERTEX_MAP)4. Missing validation: Empty string handling
5. No default model validation in frontend
const defaultModel = clusterModels.find((m) => m.default)?.name
?? clusterModels[0]?.name // Use first model if no default
?? "claude-sonnet-4-5"; // Final fallback🔵 Minor Issues6. Python: Dictionary comprehension could filter models more explicitly
vertex_map = {}
for m in models:
if "vertexId" not in m or not m["vertexId"]:
logger.info(f"Skipping model without vertexId: {m.get('name', 'unknown')}")
else:
vertex_map[m["name"]] = m["vertexId"]
return vertex_map7. ConfigMap comments could be more descriptive
# Available models for the platform (consumed by backend, operator, runner)
# Format: [{"name":"...","displayName":"...","vertexId":"...","default":true}]
# Required: name, displayName | Optional: vertexId (for Vertex AI), default (boolean)
MODELS_JSON: '...'8. No error boundary for model dropdown rendering
const models = clusterModels.length > 0 && clusterModels.every(m => m.name && m.displayName)
? clusterModels.map((m) => ({ value: m.name, label: m.displayName }))
: fallbackModels;Positive Highlights✅ Excellent fallback patterns - All three components gracefully degrade to hardcoded defaults RecommendationsPre-Merge (Priority Order)
Post-Merge Enhancements
Testing Checklist ProgressBased on PR description test plan:
Adherence to Project StandardsBackend (Go):
Frontend (TypeScript/React):
Security:
Overall: 8/10 - Solid implementation, needs type safety and reactive default handling improvements before merge. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
1 participant
Summary
MODELS_JSONkey to the existingoperator-configConfigMap (all overlays: kind, e2e, production, local-dev, minikube)MODELS_JSONenv var and includesmodelsarray inGET /api/cluster-inforesponseuseClusterInfo()hook and populates the session creation dropdown dynamically (falls back to hardcoded list if empty)MODELS_JSONthrough to runner pods as an env varVERTEX_MODEL_MAPfromMODELS_JSONat startup (falls back to hardcoded map)Adding or removing a model is now: edit ConfigMap →
kubectl rollout restart→ done. No code changes or image rebuilds needed.Test plan
GET /api/cluster-inforeturnsmodelsarray populated from ConfigMapdefault: trueflagmodelsis emptyMODELS_JSONenv varMODELS_JSONis unsetgo vet,gofmt,tsc --noEmit,next buildall pass🤖 Generated with Claude Code