Wire OH_LLM_MODEL_KIND to enable DB-backed verified models in SaaS#678
Conversation
Wires up the SaaS verified-models DI injector so the OpenHands app reads its verified model list from the database (maintained via the admin API) instead of falling back to the hardcoded list shipped in openhands-sdk. Before OpenHands/OpenHands#14237, enterprise/saas_server.py called override_llm_models_dependency() unconditionally. That PR replaced the mechanism with a generic LLMModelServiceInjector whose default is DefaultLLMModelService (hardcoded). The SaaS variant (SaaSLLMModelServiceInjector) is now only registered when OH_LLM_MODEL_KIND points at it. Production was therefore silently serving the hardcoded model list and admin-API changes did not show up in app.all-hands.dev. Mirrors the pattern already used for OH_APP_CONVERSATION_INFO_KIND. Fixes #677 Co-authored-by: openhands <openhands@all-hands.dev>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
The fix is correct and well-motivated. After OpenHands/OpenHands#14237 removed the unconditional SaaSLLMModelServiceInjector registration and replaced it with a DI-based approach, OH_LLM_MODEL_KIND must be set at the chart level to restore DB-backed verified-model behavior for any cluster that does not layer a deploy-repo override on top. The placement immediately after OH_APP_CONVERSATION_INFO_KIND is natural and consistent with the existing SaaS env block pattern.
One concern: chart version appears not bumped. The PR checklist says 0.7.34 → 0.7.35, but no Chart.yaml change appears in the diff — main already carries version: 0.7.35 from a prior merge. This env-var addition changes the chart's runtime behavior and should be tracked under a fresh semver (0.7.36). Without a bump, Helm upgrade pipelines and OCI artifact registries won't surface a new chart artifact for this change, which could silently leave clusters on the old behavior.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| - name: OH_APP_CONVERSATION_INFO_KIND | ||
| value: server.utils.saas_app_conversation_info_injector.SaasAppConversationInfoServiceInjector | ||
| - name: OH_LLM_MODEL_KIND | ||
| value: server.verified_models.verified_model_router.SaaSLLMModelServiceInjector |
There was a problem hiding this comment.
🟠 Important: The version is on both and this branch, so this functional change will ship under the same semver as the previous release. Please bump the chart version to (or the next appropriate version) so that Helm can detect the change and artifact registries publish a distinct chart version.
raymyers
left a comment
There was a problem hiding this comment.
CI will probably force a chart version bump, but otherwise looks good
Description
Fixes #677.
Adds
OH_LLM_MODEL_KINDto the default SaaS env block incharts/openhands/templates/_env.yamlso that the OpenHands app pod reads its verified-model list from theverified_modelsDB table (maintained via the admin API) instead of falling back to the hardcoded model list shipped inopenhands-sdk.Background — the regression
Before OpenHands/OpenHands#14237,
enterprise/saas_server.pyregistered the SaaS verified-models dependency unconditionally:That PR removed the unconditional registration and replaced it with a generic DI-based
LLMModelServiceInjectorwhose default isDefaultLLMModelService(hardcoded). The SaaS variant (SaaSLLMModelServiceInjector) is now only wired in whenOH_LLM_MODEL_KINDis set to point at it.The staging/production deploy repo already set this env var via OpenHands/deploy#4287, but staging consumes that value successfully while production (
app.all-hands.dev) was still serving the hardcoded list — because for any cluster that does not also layer a deploy-repo override on top of this chart, the env var simply wasn’t set. By baking it into the SaaS defaults in the chart itself, both prod-style and self-hosted SaaS installations get the correct, DB-backed catalog out of the box.This mirrors the existing pattern already used for
OH_APP_CONVERSATION_INFO_KINDimmediately above it.Helm Chart Checklist
versionfield inChart.yamlfor each modified chart (openhands:0.7.34→0.7.35)OH_APP_CONVERSATION_INFO_KIND).Values.envstill take precedence per the dedup logic inopenhands.env)Additional Notes
GET /api/v1/config/models/search?provider__eq=openhands&query=<a-db-only-model>should return entries that exist only in theverified_modelstable (e.g. models added/removed dynamically via the admin API), instead of stopping at the SDK-hardcoded list.This PR was created by an AI agent (OpenHands) on behalf of @juanmichelini.
@juanmichelini can click here to continue refining the PR