Tolerate comma-separated values in LiteLLM model/deployment config fields

[ak684]

Problem

The bundled-LiteLLM model_list and LITELLM_DEFAULT_MODEL templates parse the model/deployment textareas (anthropic_models, vertex_models, custom_models, azure_deployments, bedrock_model_ids) by splitting on newlines only (splitList "\n").

If an admin enters the models comma-separated on a single line — e.g. claude-sonnet-4-5-20250929, claude-opus-4-7 — the whole string is treated as one model whose model_name is the literal "claude-sonnet-4-5-20250929, claude-opus-4-7". No model named claude-sonnet-4-5-20250929 then exists, so every conversation fails with an opaque proxy error and no signal at config time:

litellm.BadRequestError: Invalid model name passed in model=claude-sonnet-4-5-20250929.
Call /v1/models to view available models for your key.

This is a silent, total, hard-to-diagnose failure (surfaces deep in the agent runtime, not at the config field). Comma-separating a list is a natural habit, and it was hit on a real install whose anthropic_models was set to claude-sonnet-4-5-20250929, claude-opus-4-7.

Fix

Normalize commas to newlines before splitting — append | replace "," "\n" to every *_models / *_deployments read (14 read-points across base, embedded_postgres, and HasLocalRegistry blocks). The existing trim + dedupe handle whitespace and repeats.

• Newline input: unchanged.
• Comma or mixed input: now yields correct separate entries.
• No model ID contains a comma, so splitting on , is safe.

Verification

Rendered the identical sprig pipeline via helm template for both input forms:

Input	Result
claude-sonnet-4-5-20250929, claude-opus-4-7 (comma, 1 line)	model_count: 2 → two correct entries
claude-sonnet-4-5-20250929\nclaude-opus-4-7 (newline)	model_count: 2 (backward-compatible)

Notes

• Changes are confined to replicated/openhands.yaml (not under charts/), so no chart-version bump is required.
• The field help text still recommends one-per-line; the parser is now simply forgiving of the common comma mistake. A follow-up could add config-time validation for a clearer message, but this removes the real-world failure.

🤖 Generated with Claude Code

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

🔴 Taste Rating: Needs improvement

[CRITICAL ISSUES]

• [replicated/openhands.yaml, Line 727] Incomplete parser coverage: The PR normalizes the LiteLLM proxy model_list/LITELLM_DEFAULT_MODEL readers, but the same textarea-derived first-model readers in replicated/secrets.yaml still split only on newlines for azure_deployment_name, bedrock_model_id, and custom_model. Those values feed the legacy LLM_MODEL/credential fallback secrets, so comma-separated input can still render a single invalid model/deployment string in part of the install. Apply the same normalization there, or centralize the parsing so all rendered artifacts agree.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM
  This is a small, pattern-following Replicated template change with no dependency or API surface changes, but it touches self-hosted LLM routing defaults. Misrendering these values can break all conversations for affected installs, and the remaining inconsistent parser keeps that risk alive for fallback secret paths.

VERDICT:
❌ Needs rework: The main proxy config path is moving in the right direction, but all consumers of the same textarea fields need the same comma normalization before this is safe to approve.

KEY INSIGHT:
Parsing policy for these model/deployment textareas is now duplicated; every derived value must share the same normalization or the rendered install becomes internally inconsistent.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

[ak684]

Good catch on the incomplete coverage — addressed in 3e46e7b.

replicated/secrets.yaml had the same pattern in three first-value readers (azure_deployment_name, bedrock_model_id, custom_model) that derive the first model from the per-provider textareas and feed the legacy LLM_MODEL fallback secrets. They now get the same | replace "," "\n" normalization, so the proxy model_list, LITELLM_DEFAULT_MODEL, and the fallback secrets all parse identically.

Verified via helm template of the identical sprig pipelines:

• model_list (comma input) → 2 separate entries; (newline) → 2 (unchanged)
• first-value reader (comma input) → first model claude-sonnet-4-5-20250929, not the joined string; (newline) → same

On "centralize the parser": KOTS config templating evaluates each repl{{ }} expression inline with no shared named-template/include mechanism for ConfigValues, so per-read normalization is the practical approach here; all read-points now use the identical replace "\r" "\n" | replace "," "\n" prefix.

[all-hands-bot]

✅ Review complete.

Posted an APPROVE review: the follow-up commit 3e46e7b closes the parser-coverage gap from the prior review, and all 17 read-points across replicated/openhands.yaml and replicated/secrets.yaml now use the same | replace "," "\n" normalization.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

🟢 Taste Rating: Good taste

The follow-up commit 3e46e7b closes the only critical issue from the prior review: replicated/secrets.yaml now applies the same | replace "," "\n" normalization to azure_deployment_name (line 61), bedrock_model_id (line 68), and custom_model (line 71). All 17 read-points across both files parse comma-separated textarea input consistently, so no rendered artifact can drift from the others.

Verified the sprig pipeline (splitList -> replace "," "\n" -> trim -> first non-empty) with a Python simulation: single-line comma input now yields the first model cleanly, newline-only input is unchanged, and edge cases (leading/trailing whitespace, blank lines, trailing commas, mixed CRLF) all behave correctly.

Backward compatibility is preserved (the new filter is purely additive — a , in any of these values would already have rendered an invalid single model name), and the supported providers do not emit model IDs containing commas.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  Small, pattern-following Replicated template change. No API or schema surface changes; no dependency bumps; CI (lint and check-secret-checksum) passes on the head commit. Worst-case regression is "behaves exactly like before" for newline-only input.

VERDICT:
✅ Worth merging: Minimal, complete fix for a real silent-failure mode; previous parser-coverage gap is now closed.

KEY INSIGHT:
The first-model parser is still duplicated across 17 read-points (unchanged by this PR); a follow-up could centralize it into a single Replicated helper template/macro to make future parser changes mechanically consistent.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation