Skip to content

fix(inventory): handle dict entries in provider models lists for model selector - #57411

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-57405-model-selector-dict-crash
Closed

fix(inventory): handle dict entries in provider models lists for model selector#57411
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-57405-model-selector-dict-crash

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a crash in the model selector dropdown when a provider's models: config contains dict entries instead of plain strings. When users hand-edit config.yaml with per-model metadata (e.g. models: [{id: "gpt-4", context_length: 128000}]), the dicts leak into the model list. Downstream dedup logic in build_models_payload() calls .lower() on every entry, causing AttributeError: 'dict' object has no attribute 'lower'.

The fix extracts the id field from dict entries at the source (list_authenticated_providers() Sections 3 and 4) and adds defensive type guards in the inventory dedup path.

Related Issue

Fixes #57405

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/model_switch.py: In Section 3 (providers: handling) and Section 4 (custom_providers: handling), when cfg_models is a list, extract m.get("id") from dict entries instead of appending the raw dict to the models list.
  • hermes_cli/inventory.py: Add isinstance(m, str) guard in build_models_payload() dedup logic (lines 188 and 210) so non-string entries are filtered out instead of crashing.
  • tests/hermes_cli/test_model_dict_entries.py: 5 regression tests covering list-of-dicts, flat string list, keyed dict, custom_providers list-of-dicts, and dicts without id field.

How to Test

  1. Add a providers: entry in ~/.hermes/config.yaml with models: as a list of dicts:
    providers:
      test:
        base_url: https://example.com/v1
        models:
          - id: model-a
            context_length: 128000
  2. Open the Desktop model selector — it should load without crashing.
  3. Run pytest tests/hermes_cli/test_model_dict_entries.py -v — all 5 tests should pass.
  4. Verified locally: build_models_payload() returns all-string model entries with dict-in-list config.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS (Apple Silicon)

Documentation and Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Reproduction test output:

# Before fix: AttributeError: 'dict' object has no attribute 'lower'
# After fix:
PASS: test-provider models = ['model-a', 'model-b']
PASS: build_models_payload OK (38 providers)
PASS: string list models still work

…l selector

When a provider's `models:` config is a list of dicts (hand-edited
config or legacy format with per-model metadata like context_length),
the dicts were appended directly to the models list. Downstream code
in `build_models_payload` calls `.lower()` on every model entry for
deduplication, causing an `AttributeError: 'dict' object has no
attribute 'lower'` crash in the Desktop model selector.

Fix: extract the `id` field from dict entries in both Section 3
(providers:) and Section 4 (custom_providers:) of
`list_authenticated_providers()`, and add defensive type guards in
`build_models_payload()` dedup logic.

Regression tests cover: list-of-dicts, flat string list, keyed dict,
custom_providers list-of-dicts, and dicts without 'id' field.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard comp/desktop Electron desktop app (apps/desktop/*) area/config Config system, migrations, profiles P3 Low — cosmetic, nice to have labels Jul 3, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #57405 (the issue this fixes), and the competing open fix PRs #33372 (earliest — _add_model_id() helper in model_switch.py + tests) and #55770 (narrow inline model_switch.py list-branch fix). This PR is the broadest of the three: it is the only one that also guards build_models_payload() in hermes_cli/inventory.py — the exact .lower() crash site referenced in #57405 — in addition to the model_switch.py source fix, plus 5 regression tests. Flagging the cluster so a maintainer can pick one (breadth argues for this PR; earliest is #33372).

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused reproduction and regression coverage. This is an automated hermes-sweeper review; current main already provides the requested normalization.

  • fc18d15f4 (fix: preserve static custom provider models) added hermes_cli/model_switch.py:_declared_model_ids() at line 55. It converts list-of-dict id/name entries into validated string IDs.
  • The affected providers: picker construction uses that helper at hermes_cli/model_switch.py:2046; the grouped custom_providers: path uses it at line 2274.
  • Current regression coverage uses models: [{"id": ...}] and verifies the shared inventory payload at tests/hermes_cli/test_inventory.py:65 and :728.
  • The implementation is included in release v2026.7.7.

The related issue discussion also identifies this PR as part of the competing-fix cluster. Closing as implemented on main.

@teknium1 teknium1 closed this Jul 15, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Model selector crashes: 'dict' object has no attribute 'lower'

3 participants