From 84173197115e104ef8b4036c93e66ac10aa7b398 Mon Sep 17 00:00:00 2001 From: bussyjd Date: Wed, 29 Apr 2026 00:07:11 +0800 Subject: [PATCH] fix(model): unify LiteLLM model_name contract, remove double-strip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `obol model setup custom`, the LiteLLM `model_name` convention, and the agent-side stripProviderPrefix helpers were tangled in a way that quietly broke flow-14 with a 400 "no healthy deployments for this model" on every chat-completion against a custom vLLM endpoint: 1. AddCustomEndpoint wrote `model_name: custom//`. 2. hermes.configuredModels saw it, called rankModels which pre-stripped to `/` before delegating to model.Rank. 3. model.Rank also strips internally for ranking heuristics — but returns the original string. With the pre-strip from (2) the "original" was already mutilated. 4. configuredModels then ran stripProviderPrefix on the primary AGAIN before returning, leaving the agent calling LiteLLM with bare `` while only `custom//` was registered. The band-aid in ca820c9 dropped the `custom//` prefix on writes, which unblocked the flow but left the underlying double-strip surface intact. This change picks the contract explicitly: LiteLLM `model_name` is the bare model identifier — the agent reads it straight back as the `model` field on chat-completion calls and must round-trip unchanged. Same convention every other code path already uses (Ollama, Anthropic, OpenAI explicit entries). Implementation: - internal/model/model.go: extract buildCustomEndpointEntry, document the contract on AddCustomEndpoint, drop the leftover `_ = name` bookkeeping. - internal/model/rank.go: keep the unexported stripProviderPrefix for ranking heuristics, add a doc comment explicitly forbidding its use on round-trippable identifiers. - internal/hermes/hermes.go: delete stripProviderPrefix / stripProviderPrefixes; rankModels now passes through to model.Rank without pre-stripping; configuredModels returns the LiteLLM model list unchanged. The agent's `model.default` is now byte-identical to the LiteLLM ConfigMap entry. - cmd/obol/model.go: clarify --name flag help to "informational only" — it still surfaces in `obol model status` but does not participate in the route key. Tests: - internal/model/rank_test.go: TestRank_PreservesProviderPrefixOnOutput pins the round-trip property at the Rank() boundary, including the legacy `custom//` shape. - internal/model/model_test.go: TestBuildCustomEndpointEntry covers the bare-model_name + openai/-routing shape, the empty-key fallback, and that colon-tagged ids survive intact. - internal/hermes/rankmodels_test.go: rewritten to assert the contract (was asserting the now-removed strip). Adds the `custom//` regression guard. - internal/hermes/hermes_test.go: TestGenerateConfig_PrimaryIsRoundTrippable covers the end-to-end shape — whatever LiteLLM publishes is what the agent sends back. Refs ca820c9 (band-aid). --- cmd/obol/model.go | 4 +- internal/hermes/hermes.go | 50 ++++++++------------ internal/hermes/hermes_test.go | 45 ++++++++++++++++++ internal/hermes/rankmodels_test.go | 50 +++++++++++++++++--- internal/model/model.go | 76 ++++++++++++++++++------------ internal/model/model_test.go | 51 ++++++++++++++++++++ internal/model/rank.go | 11 +++++ internal/model/rank_test.go | 39 +++++++++++++++ 8 files changed, 257 insertions(+), 69 deletions(-) diff --git a/cmd/obol/model.go b/cmd/obol/model.go index 0e2eb0a2..726285fa 100644 --- a/cmd/obol/model.go +++ b/cmd/obol/model.go @@ -240,9 +240,9 @@ func modelSetupCustomCommand(cfg *config.Config) *cli.Command { Name: "custom", Usage: "Add a custom OpenAI-compatible endpoint (validates before adding)", Flags: []cli.Flag{ - &cli.StringFlag{Name: "name", Usage: "Short name for the endpoint (e.g. my-vllm)", Required: true}, + &cli.StringFlag{Name: "name", Usage: "Short label for the endpoint (informational only — LiteLLM keys the route by --model, not --name)", Required: true}, &cli.StringFlag{Name: "endpoint", Usage: "Full base URL (e.g. http://host:8000/v1)", Required: true}, - &cli.StringFlag{Name: "model", Usage: "Model name at the endpoint", Required: true}, + &cli.StringFlag{Name: "model", Usage: "Model identifier at the endpoint — this is also the LiteLLM model_name the agent will call", Required: true}, &cli.StringFlag{Name: "api-key", Usage: "API key (optional, some endpoints don't require it)"}, &cli.BoolFlag{Name: "no-sync", Usage: "Skip the agent model sync (batch with other model commands, then run `obol model sync` once)"}, }, diff --git a/internal/hermes/hermes.go b/internal/hermes/hermes.go index 525fd2ff..876af18d 100644 --- a/internal/hermes/hermes.go +++ b/internal/hermes/hermes.go @@ -987,11 +987,20 @@ func syncObolSkills(cfg *config.Config, id string) error { return nil } +// configuredModels returns the agent-facing model list and the primary model +// name. Both are returned as round-trippable LiteLLM `model_name` strings: +// the agent passes `primary` back as the `model` field on chat-completion +// calls, and LiteLLM matches by exact string against the entries in the +// returned slice. NO provider-prefix stripping happens on this path — +// LiteLLM `model_name` is the contract identifier end-to-end. +// +// See internal/model/model.go (AddCustomEndpoint, buildModelEntries) for +// where the bare-name convention is written into the LiteLLM ConfigMap. func configuredModels(cfg *config.Config, u *ui.UI) ([]string, string, error) { models, err := model.GetConfiguredModels(cfg) if err == nil && len(models) > 0 { primary, _ := rankModels(models) - return stripProviderPrefixes(models), stripProviderPrefix(primary), nil + return models, primary, nil } ollamaModels, ollamaErr := model.ListOllamaModels() @@ -1024,7 +1033,7 @@ func configuredModels(cfg *config.Config, u *ui.UI) ([]string, string, error) { } primary, _ := rankModels(names) - return names, stripProviderPrefix(primary), nil + return names, primary, nil } func generateConfig(cfg *config.Config, primary string) ([]byte, error) { @@ -1183,38 +1192,17 @@ func litellmMasterKey(cfg *config.Config) string { return "sk-obol-" + strings.TrimSpace(string(data)) } -func stripProviderPrefix(modelName string) string { - modelName = strings.TrimSpace(strings.Trim(modelName, `"'`)) - if before, after, ok := strings.Cut(modelName, "/"); ok && before != "" && after != "" { - return after - } - return modelName -} - -func stripProviderPrefixes(modelNames []string) []string { - if len(modelNames) == 0 { - return nil - } - - out := make([]string, 0, len(modelNames)) - for _, name := range modelNames { - if trimmed := stripProviderPrefix(name); trimmed != "" { - out = append(out, trimmed) - } - } - return out -} - // rankModels delegates to model.Rank, which knows how to prefer larger local // models and frontier cloud models. Kept as a thin wrapper so call sites -// don't need to import internal/model directly and to preserve the existing -// stripProviderPrefix shape on the inputs. +// don't need to import internal/model directly. +// +// IMPORTANT: do NOT pre-strip provider prefixes here. model.Rank strips +// internally for ranking heuristics but returns the ORIGINAL strings so the +// agent can round-trip them back to LiteLLM. Stripping at this layer would +// break that round-trip — that's exactly the double-strip bug that +// ca820c9 worked around for custom endpoints. func rankModels(models []string) (primary string, fallbacks []string) { - stripped := make([]string, len(models)) - for i, m := range models { - stripped[i] = stripProviderPrefix(m) - } - return model.Rank(stripped) + return model.Rank(models) } func k3dNodeExec(cfg *config.Config, hostPath, shellCmd string) error { diff --git a/internal/hermes/hermes_test.go b/internal/hermes/hermes_test.go index 09d74e4d..e2a1840e 100644 --- a/internal/hermes/hermes_test.go +++ b/internal/hermes/hermes_test.go @@ -23,6 +23,51 @@ func testConfig(t *testing.T) *config.Config { return &config.Config{ConfigDir: dir, DataDir: dir, BinDir: dir} } +// TestGenerateConfig_PrimaryIsRoundTrippable guards the LiteLLM model_name +// contract end-to-end: whatever string the agent's `model.default` is set to +// MUST match a `model_name` entry in the LiteLLM ConfigMap byte-for-byte, +// because Hermes will pass it back as the `model` field on every +// chat-completion call. Stripping anywhere on this path causes the agent to +// call LiteLLM with a key that no longer matches a registered route — the +// flow-14 / ca820c9 regression. +func TestGenerateConfig_PrimaryIsRoundTrippable(t *testing.T) { + cases := []struct { + name string + primary string + }{ + {"bare ollama tag", "qwen3.5:9b"}, + {"bare claude id", "claude-opus-4-7"}, + {"bare openai id", "gpt-5.4"}, + // Wildcard-expanded entries can carry the provider prefix; the + // agent must still send back the exact string LiteLLM published. + {"provider-prefixed", "anthropic/claude-3-5-sonnet-latest"}, + // Custom endpoints write `model_name: ` after the contract + // fix; this case guards that the agent picks up that bare name + // without re-namespacing. + {"custom endpoint bare", "qwen36-fast"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + raw, err := generateConfig(testConfig(t), tc.primary) + if err != nil { + t.Fatalf("generateConfig: %v", err) + } + var cfg map[string]any + if err := yaml.Unmarshal(raw, &cfg); err != nil { + t.Fatalf("yaml.Unmarshal: %v", err) + } + modelCfg, ok := cfg["model"].(map[string]any) + if !ok { + t.Fatalf("model config missing") + } + if got := modelCfg["default"]; got != tc.primary { + t.Fatalf("model.default = %q, want %q (round-trip mismatch)", got, tc.primary) + } + }) + } +} + func TestGenerateConfig_UsesLiteLLMCustomProvider(t *testing.T) { raw, err := generateConfig(testConfig(t), "gpt-5.2") if err != nil { diff --git a/internal/hermes/rankmodels_test.go b/internal/hermes/rankmodels_test.go index 9cbbbe5b..b86e6f2a 100644 --- a/internal/hermes/rankmodels_test.go +++ b/internal/hermes/rankmodels_test.go @@ -6,13 +6,17 @@ import "testing" // from the colleague's screenshot: Hermes was deploying with `llama3.2:1b` as // the default model, which then parroted its own tool list back on every // "hello" prompt. The fix moved capability ranking into model.Rank; this test -// just confirms the Hermes-side wrapper still calls into it correctly and -// keeps the openai/-prefix-stripping shape intact. +// just confirms the Hermes-side wrapper still calls into it correctly. +// +// Contract: bare LiteLLM model_name strings come in, the SAME bare strings +// come back out — no provider-prefix stripping at this layer. The agent must +// be able to round-trip the returned primary back to LiteLLM without +// modification. func TestRankModels_HermesWrapper_PrefersLargerLocalModel(t *testing.T) { primary, fallbacks := rankModels([]string{ - "openai/llama3.2:1b", - "openai/qwen3.5:9b", - "openai/llama3.2:3b", + "llama3.2:1b", + "qwen3.5:9b", + "llama3.2:3b", }) if primary != "qwen3.5:9b" { t.Fatalf("primary: got %q, want qwen3.5:9b", primary) @@ -22,13 +26,47 @@ func TestRankModels_HermesWrapper_PrefersLargerLocalModel(t *testing.T) { } } +// TestRankModels_HermesWrapper_PrefersClaudeOverLocal exercises the cloud +// tier. Cloud entries written by buildModelEntries are bare (e.g. +// `claude-opus-4-7`, not `anthropic/claude-opus-4-7`), and the wrapper must +// preserve that. func TestRankModels_HermesWrapper_PrefersClaudeOverLocal(t *testing.T) { primary, _ := rankModels([]string{ "qwen3.5:9b", - "anthropic/claude-opus-4-7", + "claude-opus-4-7", "llama3.2:1b", }) if primary != "claude-opus-4-7" { t.Fatalf("primary: got %q, want claude-opus-4-7", primary) } } + +// TestRankModels_HermesWrapper_PreservesProviderPrefixIfPresent guards the +// round-trip property. If something upstream (a wildcard expansion, a +// hand-edited ConfigMap, an older release) writes a `provider/model` shape +// into LiteLLM, we still need to return the EXACT string so the agent's +// chat-completion call matches by literal string. Stripping at this layer +// was the double-strip bug fixed in ca820c9 — this test guards against +// reintroducing it. +func TestRankModels_HermesWrapper_PreservesProviderPrefixIfPresent(t *testing.T) { + primary, _ := rankModels([]string{ + "anthropic/claude-opus-4-7", + "openai/gpt-4o", + "qwen3.5:9b", + }) + if primary != "anthropic/claude-opus-4-7" { + t.Fatalf("primary: got %q, want anthropic/claude-opus-4-7 (unstripped)", primary) + } +} + +// TestRankModels_HermesWrapper_CustomNamespacedEntryRoundTrips guards the +// specific shape that broke flow-14: a legacy `custom//` entry +// that double-stripping would mangle to ``, leaving the agent calling +// LiteLLM with a key that no longer matched the registered route. +func TestRankModels_HermesWrapper_CustomNamespacedEntryRoundTrips(t *testing.T) { + in := []string{"custom/spark1-vllm/qwen36-fast"} + primary, _ := rankModels(in) + if primary != in[0] { + t.Fatalf("primary: got %q, want %q (must round-trip unchanged)", primary, in[0]) + } +} diff --git a/internal/model/model.go b/internal/model/model.go index 6ff2cf28..ce910fb7 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -630,6 +630,22 @@ func RemoveModel(cfg *config.Config, u *ui.UI, modelName string) error { // AddCustomEndpoint adds a custom OpenAI-compatible endpoint to LiteLLM // after validating it works. +// +// LiteLLM `model_name` contract — the canonical identifier is the bare +// `modelName`. Same convention every other code path in this stack uses: +// Ollama writes `qwen3.5:9b`, Anthropic writes `claude-opus-4-7`, OpenAI +// writes `gpt-5.4`. The agent (Hermes / OpenClaw) reads `model_name` straight +// back as the `model` field on chat-completion calls — any provider-prefix +// namespacing (`custom//`) on this side breaks that round-trip +// because the agent then strips it and calls LiteLLM with a key that doesn't +// match. +// +// The `name` arg is informational only. It is surfaced via +// `obol model status` / `list` for human reference but does NOT participate +// in the LiteLLM route key. Two custom endpoints that publish the same +// `modelName` will overwrite each other in the LiteLLM ConfigMap; that is +// the natural "repoint my model" behavior an operator running +// `obol model setup custom` wants when they re-run the command. func AddCustomEndpoint(cfg *config.Config, u *ui.UI, name, endpoint, modelName, apiKey string) error { kubectlBinary := filepath.Join(cfg.BinDir, "kubectl") kubeconfigPath := filepath.Join(cfg.ConfigDir, "kubeconfig.yaml") @@ -658,38 +674,17 @@ func AddCustomEndpoint(cfg *config.Config, u *ui.UI, name, endpoint, modelName, u.Infof("Cluster endpoint: %s (translated from %s)", clusterEndpoint, endpoint) } - // Build model entry. The LiteLLM `model_name` is the user-facing - // identifier the agent will pass on chat-completion calls. We use the - // bare `modelName` so the agent's request matches the LiteLLM entry by - // exact string — the `name` flag is still surfaced in `obol model - // status` / `list` for human reference, but LiteLLM keys the route by - // the model alone. Re-running `obol model setup custom --name X - // --model Y` with the same Y simply re-binds, which is the natural - // "repoint my model" behavior an operator wants. - // - // (The historical `custom//` namespaced ID caused - // Hermes to call LiteLLM with a stripped name that no longer matched - // the entry, surfacing as 400 "no healthy deployments for this model" - // on every agent invocation.) - litellmModel := "openai/" + modelName - modelID := modelName - _ = name // currently informational only; reserved for future multi-endpoint namespacing + entry := buildCustomEndpointEntry(modelName, clusterEndpoint, apiKey) - entry := ModelEntry{ - ModelName: modelID, - LiteLLMParams: LiteLLMParams{ - Model: litellmModel, - APIBase: clusterEndpoint, - APIKey: apiKey, - }, - } - if apiKey == "" { - entry.LiteLLMParams.APIKey = "none" + // Patch ConfigMap for persistence. The display label is logged so an + // operator can correlate the call with their `--name` arg, but it isn't + // part of the route key. + if name != "" { + u.Infof("Adding custom endpoint %q (model: %s) to LiteLLM config", name, modelName) + } else { + u.Infof("Adding custom endpoint (model: %s) to LiteLLM config", modelName) } - // Patch ConfigMap for persistence. - u.Infof("Adding custom endpoint %q to LiteLLM config", name) - if err := patchLiteLLMConfig(kubectlBinary, kubeconfigPath, []ModelEntry{entry}); err != nil { return fmt.Errorf("failed to update LiteLLM config: %w", err) } @@ -700,7 +695,7 @@ func AddCustomEndpoint(cfg *config.Config, u *ui.UI, name, endpoint, modelName, return RestartLiteLLM(cfg, u, name) } - u.Successf("Custom endpoint %q added (model: %s)", name, modelID) + u.Successf("Custom endpoint %q added (model: %s)", name, modelName) return nil } @@ -1143,6 +1138,27 @@ func buildModelEntries(provider string, models []string) []ModelEntry { return entries } +// buildCustomEndpointEntry constructs the LiteLLM ModelEntry for a custom +// OpenAI-compatible endpoint added via `obol model setup custom`. The +// `model_name` is the bare `modelName` — see the AddCustomEndpoint doc +// comment for the round-trip contract this enforces. Extracted as a +// standalone helper so the entry shape is unit-testable without going +// through the full kubectl-driven AddCustomEndpoint path. +func buildCustomEndpointEntry(modelName, clusterEndpoint, apiKey string) ModelEntry { + entry := ModelEntry{ + ModelName: modelName, + LiteLLMParams: LiteLLMParams{ + Model: "openai/" + modelName, + APIBase: clusterEndpoint, + APIKey: apiKey, + }, + } + if apiKey == "" { + entry.LiteLLMParams.APIKey = "none" + } + return entry +} + // patchLiteLLMConfig reads the current config.yaml from the ConfigMap, // merges new model entries (replacing existing by model_name), and patches back. func patchLiteLLMConfig(kubectlBinary, kubeconfigPath string, entries []ModelEntry) error { diff --git a/internal/model/model_test.go b/internal/model/model_test.go index 1a866fab..0053b230 100644 --- a/internal/model/model_test.go +++ b/internal/model/model_test.go @@ -88,6 +88,57 @@ func TestBuildModelEntries(t *testing.T) { }) } +// TestBuildCustomEndpointEntry encodes the LiteLLM `model_name` contract for +// custom endpoints added via `obol model setup custom`: +// +// - `model_name` is the BARE model identifier (no `custom//` namespace +// and no `openai/` provider prefix). The agent reads this string and +// passes it back as the `model` field on chat-completion calls; any +// namespacing here breaks the round-trip — the flow-14 / ca820c9 bug. +// - `litellm_params.model` carries the `openai/` provider hint so LiteLLM +// routes through its OpenAI-compatible adapter to the user's upstream. +// - The `name` (label) flag does NOT participate in the route key. +func TestBuildCustomEndpointEntry(t *testing.T) { + t.Run("bare model_name with openai-compat routing", func(t *testing.T) { + entry := buildCustomEndpointEntry("qwen36-fast", "http://host.k3d.internal:8000/v1", "secret-key") + if entry.ModelName != "qwen36-fast" { + t.Errorf("ModelName = %q, want bare %q (contract: bare LiteLLM model_name)", entry.ModelName, "qwen36-fast") + } + if strings.HasPrefix(entry.ModelName, "custom/") { + t.Errorf("ModelName = %q must NOT carry custom// namespace", entry.ModelName) + } + if entry.LiteLLMParams.Model != "openai/qwen36-fast" { + t.Errorf("litellm_params.model = %q, want openai/qwen36-fast", entry.LiteLLMParams.Model) + } + if entry.LiteLLMParams.APIBase != "http://host.k3d.internal:8000/v1" { + t.Errorf("api_base = %q", entry.LiteLLMParams.APIBase) + } + if entry.LiteLLMParams.APIKey != "secret-key" { + t.Errorf("api_key = %q, want secret-key", entry.LiteLLMParams.APIKey) + } + }) + + t.Run("empty api_key falls back to none", func(t *testing.T) { + // Some self-hosted OpenAI-compatible servers (vLLM, llama.cpp, mlx-lm) + // don't require auth. LiteLLM still wants a non-empty api_key field + // or the openai client errors before even hitting the endpoint. + entry := buildCustomEndpointEntry("any-model", "http://host:8000/v1", "") + if entry.LiteLLMParams.APIKey != "none" { + t.Errorf("api_key = %q, want %q (LiteLLM placeholder)", entry.LiteLLMParams.APIKey, "none") + } + }) + + t.Run("model with colon tag survives intact", func(t *testing.T) { + // A custom endpoint can serve a tagged model id (e.g. mlx-lm publishing + // `qwen3:9b-mlx`). The colon must NOT be stripped or interpreted as a + // provider separator. + entry := buildCustomEndpointEntry("qwen3:9b-mlx", "http://host:8000/v1", "") + if entry.ModelName != "qwen3:9b-mlx" { + t.Errorf("ModelName = %q, want qwen3:9b-mlx unchanged", entry.ModelName) + } + }) +} + func TestExpandWildcard(t *testing.T) { t.Run("uses live models when available", func(t *testing.T) { live := []string{"claude-sonnet-4-6", "claude-opus-4", "gpt-4o"} diff --git a/internal/model/rank.go b/internal/model/rank.go index 29164dbf..e5bb47a8 100644 --- a/internal/model/rank.go +++ b/internal/model/rank.go @@ -88,6 +88,17 @@ func IsCloudModel(name string) bool { return false } +// stripProviderPrefix is an internal helper for ranking only. It is NOT +// exported and MUST NOT be used to mutate model identifiers that the agent +// will pass back to LiteLLM on chat-completion calls. +// +// LiteLLM `model_name` is bare (no provider prefix) — see the contract +// documented on AddCustomEndpoint and buildModelEntries. The only place a +// `provider/model` shape can sneak in is wildcard entries like `anthropic/*`, +// or legacy entries from older releases that namespaced custom endpoints as +// `custom//`. We strip those here so size/family parsing in +// IsCloudModel/cloudRank/localRank still works on tagged tokens, but the +// caller in Rank() returns the ORIGINAL string — never the stripped form. func stripProviderPrefix(name string) string { if idx := strings.Index(name, "/"); idx >= 0 { return name[idx+1:] diff --git a/internal/model/rank_test.go b/internal/model/rank_test.go index cd6a7f7b..9dece8f9 100644 --- a/internal/model/rank_test.go +++ b/internal/model/rank_test.go @@ -107,6 +107,45 @@ func TestRank_EmbeddingModelLast(t *testing.T) { } } +// TestRank_PreservesProviderPrefixOnOutput documents the contract relied on +// by internal/hermes and internal/openclaw: Rank() may use the provider +// prefix internally for ranking heuristics (e.g. detecting "claude" in +// "anthropic/claude-opus-4-7"), but it MUST return the input strings +// UNCHANGED. The agent round-trips the returned primary back to LiteLLM as +// the `model` field on chat-completions; stripping here would mismatch the +// LiteLLM model_name and surface as 400 "no healthy deployments". +func TestRank_PreservesProviderPrefixOnOutput(t *testing.T) { + cases := []struct { + name string + in []string + want string + }{ + { + "anthropic/-prefixed wins over local", + []string{"anthropic/claude-opus-4-7", "qwen3.5:9b"}, + "anthropic/claude-opus-4-7", + }, + { + "openai/-prefixed wins over local", + []string{"qwen3.5:9b", "openai/gpt-4o"}, + "openai/gpt-4o", + }, + { + "legacy custom// round-trips", + []string{"custom/spark1-vllm/qwen36-fast"}, + "custom/spark1-vllm/qwen36-fast", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + primary, _ := Rank(tc.in) + if primary != tc.want { + t.Fatalf("Rank(%v): got %q, want %q (must round-trip unchanged)", tc.in, primary, tc.want) + } + }) + } +} + func TestRank_Empty(t *testing.T) { primary, fallbacks := Rank(nil) if primary != "" || len(fallbacks) != 0 {