Skip to content

fix(models): preserve Ollama model:tag colons in parse_model_input - #56089

Open
starm2010 wants to merge 2 commits into
NousResearch:mainfrom
starm2010:fix/ollama-tag-colon-parse-model-input
Open

fix(models): preserve Ollama model:tag colons in parse_model_input#56089
starm2010 wants to merge 2 commits into
NousResearch:mainfrom
starm2010:fix/ollama-tag-colon-parse-model-input

Conversation

@starm2010

Copy link
Copy Markdown

Problem

When ACP clients (like AionUI) send custom:glm-5.2:cloud to set_session_model, parse_model_input splits at the second colon, treating the middle part (glm-5.2) as a named custom provider name. For Ollama models that use colons in their tags (e.g. glm-5.2:cloud, nomic-embed-text:latest, kimi-k2:1t-cloud), this truncates the model id to just the tag suffix (cloud instead of glm-5.2:cloud), causing HTTP 404 errors from the inference endpoint.

The existing triple-syntax guard (custom:name:model("custom:name", "model")) had no way to distinguish between:

  • custom:ollama:qwen3 — provider custom:ollama, model qwen3 (intended triple syntax)
  • custom:glm-5.2:cloud — provider custom, model glm-5.2:cloud (Ollama tag, NOT triple syntax)

Solution

Add _is_registered_custom_provider() that checks whether the middle part matches a configured custom provider in config.yaml (via get_compatible_custom_providers()). The triple split only happens when the middle part is a registered custom provider name. Otherwise, the full string after custom: is treated as the model id.

# Before (truncates Ollama tags):
if custom_name and actual_model:
    return (f"custom:{custom_name}", actual_model)

# After (only splits for registered custom providers):
if custom_name and actual_model and _is_registered_custom_provider(custom_name):
    return (f"custom:{custom_name}", actual_model)

Testing

  • Updated test_custom_triple_syntax and test_custom_triple_spaces to mock _is_registered_custom_provider (since local-server/my-server are not registered providers in the test env)
  • Added test_custom_triple_unregistered_falls_back — verifies custom:glm-5.2:cloud with a mocked False returns the full model id
  • Added test_custom_ollama_tag_not_truncated — regression test without mocks, verifies real-world Ollama model names pass through intact

Verified manually:

parse_model_input("custom:glm-5.2:cloud", "custom")    → ("custom", "glm-5.2:cloud") ✅
parse_model_input("custom:ollama:qwen3", "custom")      → ("custom:ollama", "qwen3")  ✅
parse_model_input("custom:nomic-embed-text:latest", ..) → ("custom", "nomic-embed-text:latest") ✅
parse_model_input("custom:gpt-4o", "custom")            → ("custom", "gpt-4o")         ✅

Scope

  • Only affects the triple-syntax path (custom:name:model). Single-colon path (custom:model) is unchanged.
  • No new dependencies. _is_registered_custom_provider uses get_compatible_custom_providers which already exists in hermes_cli/config.py.
  • Related: fix(acp): split custom:name/model and provider/model model names (#53545) #53558 addresses a similar parsing issue for / (slash) syntax in ACP model names. This PR is complementary (different delimiter, same root concern).

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard provider/ollama Ollama / local models P3 Low — cosmetic, nice to have labels Jul 1, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused parser fix. The reported truncation is present on current main: hermes_cli/models.py:1748-1753 unconditionally parses every nonempty custom:name:model value as a named provider, and ACP passes its selected value through that parser at acp_adapter/server.py:652-658.

Problems

  • The new parser tests mock _is_registered_custom_provider, so they do not prove the new configuration integration works. The production helper calls get_compatible_custom_providers(), which supports both legacy custom_providers and v12 keyed providers entries (hermes_cli/config.py:4910-4957). A mismatch in its name/provider_key lookup would silently break the documented named-provider syntax.

Suggested changes

  • Add one temp-HERMES_HOME integration test with a real configured named provider, asserting custom:<name>:<model> selects it and an unregistered colon-tag input remains custom plus the full model id.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@starm2010

Copy link
Copy Markdown
Author

Thanks for the review — great catch. The unit tests indeed mocked _is_registered_custom_provider, which meant the integration with get_compatible_custom_providers() and the real config.yaml path was never exercised.

I've added 3 integration tests (TestParseModelInputIntegration) that use a real HERMES_HOME with config.yaml containing named custom providers — no mocks on _is_registered_custom_provider:

  1. test_named_custom_provider_resolves: config.yaml with a custom_providers list entry named ollama-local. Asserts custom:ollama-local:qwen3-coder("custom:ollama-local", "qwen3-coder") — the full get_compatible_custom_providers → load_config → config.yaml path runs.

  2. test_unregistered_name_keeps_full_model_id: empty custom_providers list. Asserts custom:glm-5.2:cloud("custom", "glm-5.2:cloud") — the Ollama tag colon is preserved, not truncated.

  3. test_named_provider_via_providers_key: uses the v12+ keyed providers: schema (not legacy custom_providers). Verifies get_compatible_custom_providers() discovers the named provider from the keyed dict too.

All 19 tests pass (16 original unit + 3 new integration).

When ACP clients (like AionUI) send custom:glm-5.2:cloud to set_session_model,
parse_model_input would split at the second colon treating the middle part
as a named custom provider name. For Ollama models that use colons in their
tags (e.g. glm-5.2:cloud, nomic-embed-text:latest), this truncated the model
id to just the tag suffix (e.g. 'cloud' instead of 'glm-5.2:cloud'), causing
HTTP 404 errors from the inference endpoint.

The fix adds _is_registered_custom_provider() to check whether the middle
part actually matches a configured custom provider in config.yaml before
splitting. If it doesn't match, the full string after 'custom:' is treated
as the model id.

This only affects the triple-syntax path (custom:name:model). The existing
single-colon path (custom:model) is unchanged.
…onfig

Addresses sweeper review on PR NousResearch#56089: the unit tests mock
_is_registered_custom_provider, so they don't prove the configuration
integration works. These integration tests use a real HERMES_HOME
with config.yaml containing named custom providers (both legacy
custom_providers list and v12+ keyed providers dict) and verify:

1. custom:<name>:<model> selects the named custom provider when it
   exists in config.yaml's custom_providers list
2. custom:glm-5.2:cloud (unregistered middle part) keeps the full
   model id including the colon, not truncating to just 'cloud'
3. The v12+ keyed 'providers' schema is also discoverable by
   _is_registered_custom_provider via get_compatible_custom_providers

No mocks on _is_registered_custom_provider — the full
get_compatible_custom_providers -> load_config -> config.yaml path
is exercised end-to-end.
@starm2010
starm2010 force-pushed the fix/ollama-tag-colon-parse-model-input branch from 4b5d655 to 0901f3b Compare July 16, 2026 14:35
@starm2010

Copy link
Copy Markdown
Author

Friendly ping — the integration tests requested in the sweeper review (TestParseModelInputIntegration with a real HERMES_HOME + config.yaml, no mocks on _is_registered_custom_provider) have been pushed. All 19 tests pass (16 unit + 3 integration). Would appreciate a re-review to confirm the get_compatible_custom_providers()load_configconfig.yaml path is now properly covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have provider/ollama Ollama / local models sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants