Skip to content

refactor(inventory): extract shared ConfigContext + build_models_payload#23666

Closed
kshitijk4poor wants to merge 1 commit into
NousResearch:mainfrom
kshitijk4poor:refactor/inventory-context
Closed

refactor(inventory): extract shared ConfigContext + build_models_payload#23666
kshitijk4poor wants to merge 1 commit into
NousResearch:mainfrom
kshitijk4poor:refactor/inventory-context

Conversation

@kshitijk4poor

@kshitijk4poor kshitijk4poor commented May 11, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Three call-sites in the codebase each duplicated the same config-slice + list_authenticated_providers + post-processing pattern:

  • hermes_cli/web_server.py /api/model/options (dashboard)
  • tui_gateway/server.py model.options JSON-RPC
  • tui_gateway/server.py model.save_key JSON-RPC

This consolidates them onto a new hermes_cli/inventory.py:

load_picker_context() -> ConfigContext
    # Replaces the 17-LOC config-slice (model.{default,name,provider,
    # base_url}, providers:, custom_providers:) every consumer did inline.

ConfigContext.with_overrides(*, current_provider=, current_model=,
                             current_base_url=) -> ConfigContext
    # Truthy-only overlay for TUI agent-session state on top of disk
    # config. Empty getattr(agent, ...) attrs MUST NOT clobber disk.

build_models_payload(ctx, *, include_unconfigured=False,
                     picker_hints=False, canonical_order=False,
                     max_models=50) -> dict
    # Single payload builder. Delegates curation to
    # list_authenticated_providers (does not call provider_model_ids
    # per row — that pulls non-agentic models like TTS/embeddings).
    # picker_hints + canonical_order produce the TUI shape;
    # defaults match the dashboard's existing /api/model/options
    # contract.
Site Before After
/api/model/options 30-line inline config dance + raw cfg.get("custom_providers") (which misses v12+ keyed providers — see Bug A below) + list_authenticated_providers build_models_payload(load_picker_context(), max_models=50)
TUI model.options 17-line config dance + list_authenticated_providers + 45-line canonical-merge / picker-hint post-processing build_models_payload(ctx, include_unconfigured=True, picker_hints=True, canonical_order=True, max_models=50) with with_overrides() for agent-session state
TUI model.save_key Inline list_authenticated_providers + slug match build_models_payload(ctx, picker_hints=True) then slug match

Scope: 3 of 5 inventory call-sites

Issue #23359 named four call-sites (plus one this PR found in gateway/run.py). This PR migrates 3 of 5:

Call-site Status
hermes_cli/web_server.py /api/model/options ✅ migrated
tui_gateway/server.py model.options ✅ migrated
tui_gateway/server.py model.save_key ✅ migrated
cli.py:6294 interactive picker ⏭ left on legacy path (interactive flow, lower duplication-removal value)
hermes_cli/main.py:2269 aux-task switcher ⏭ left on legacy path
gateway/run.py:8999 gateway model switcher ⏭ left on legacy path

The remaining 3 call-sites would migrate cleanly to build_models_payload() in a follow-up PR; they're stable enough that there's no urgency, and bundling everything would expand this PR's scope past what reviewers can reasonably check in one pass.

One latent bug fixed; one regression guard added

  1. Bug A (real, fixed in 3 sites): web_server.py:998, tui_gateway/server.py:5178 (model.options), and tui_gateway/server.py:5312 (model.save_key) all read cfg.get("custom_providers") directly on main, missing the v12+ keyed providers: form. Routing all 3 sites through load_picker_context() (which calls get_compatible_custom_providers()) fixes the dashboard + both TUI handlers in one go.

  2. Regression guard against canonical-slug demotion (preventive, not curative): Section 3 of list_authenticated_providers sets is_user_defined=True on rows from the providers: config dict even when the slug is canonical. If a future refactor naively keys row ordering on is_user_defined, canonical providers would silently sink to the picker tail. Current tui_gateway/server.py:5194 already keys on slug membership correctly (landed in commit c23c7c994 "fix(tui): address remaining review feedback — ordering and digit shortcuts"). _reorder_canonical preserves that invariant in the new module, and test_canonical_order_uses_slug_not_is_user_defined_flag locks it as a regression guard. The bug doesn't currently exist; this prevents it from being reintroduced.

Why this PR (vs #23369)

This replaces the rejected #23369, which bundled this consolidation with three new scriptable CLI surfaces (hermes models list, hermes models status, hermes providers list), an offline-mode synthesis path, a versioned JSON contract, and text renderers — none of which have external user demand. That PR was +1638/-140; only ~230 LOC of it served the consolidation goal.

This PR is just the refactor. The scriptable CLI is deferred until a real user asks. Issue #23359 stays open after this lands — the consolidation half is addressed; the scriptable-CLI half is deferred pending external demand.

Stats

What this PR is NOT doing

  • Not adding new CLI commands or JSON contracts.
  • Not adding offline mode (--offline) — --no-live already covers cron/CI use cases via curated lists.
  • Not changing list_authenticated_providers itself — the substrate is unchanged; this only consolidates how three callers invoke it.
  • Not touching the interactive picker (cli.py), the aux-task switcher (hermes_cli/main.py), or the gateway model switcher (gateway/run.py).
  • Not addressing tracking: provider/model inventory has no scriptable surface — five PRs reinvent it, four issues blocked #23359's "scriptable inventory" framing — that's deferred until external demand surfaces.

Test plan

  • 18 new tests in tests/hermes_cli/test_inventory.py lock the public API contract:
    • load_picker_context() reproduces the inline 17-LOC config-slice exactly (full dict, name fallback, legacy bare-string model, empty config).
    • with_overrides() is truthy-only (empty agent attrs preserved).
    • build_models_payload() returns the expected {providers, model, provider} shape and does NOT call provider_model_ids per row.
    • include_unconfigured=True appends CANONICAL_PROVIDERS skeleton rows without duplicating already-present slugs.
    • picker_hints=True adds authenticated/auth_type/key_env/warning and the api_key warning format references the actual env var (e.g. ANTHROPIC_API_KEY).
    • Regression guard: canonical_order keys on slug membership, NOT is_user_defined — canonical slugs configured via the keyed providers: schema must NOT be demoted to the tail.
    • End-to-end credential-leak canary: OPENROUTER_API_KEY / ANTHROPIC_API_KEY values never appear in the JSON-serialized payload, even with picker_hints=True.
    • Frontend shape compatibility: every row exposes the keys web/src/components/ModelPickerDialog.tsx reads (name, slug, models, total_models, is_current, authenticated).
  • Live invocation verified:
    • web_server.get_model_options() → 9 authenticated rows, clean shape, current_provider/current_model populated correctly.
    • TUI model.options JSON-RPC → 36 rows (full universe with hints + canonical order), warning hint correctly formatted.
  • Regression sweep on tests/hermes_cli/ + tests/test_tui_gateway_server.py: 4379 passed, 17 failed. The 17 failures reproduce identically on origin/main (gateway systemd / WSL detection / custom-provider grouping / plugin discovery / web-server auth / hangup protection — all pre-existing, none introduced by this PR).
  • Lint diff: ruff clean. ty clean for hermes_cli/inventory.py.

Known test gaps (acknowledged, not blockers)

  • No parity test asserting build_models_payload output equals the OLD inline pipeline output for shared inputs. Behavioral coverage is solid via individual feature tests; if the inline slice had a quirk (e.g. or "" precedence on an unusual model dict), parity tests would catch it. Worth adding in a follow-up if reviewers want belt-and-braces.
  • No directed test for model.save_key post-save populated-models contract. The credential-leak canary exercises the real curation path but doesn't assert the freshly-keyed provider has non-empty models. Low risk in practice (env var is set into os.environ before the rebuild call) but uncovered in tests.
  • The "no provider_model_ids per row" assertion runs with default flags, not the production TUI shape (include_unconfigured=True, picker_hints=True, canonical_order=True). Risk is bounded because _apply_picker_hints and _reorder_canonical are pure transformations, but a one-line additional assertion would harden it.

Refs

Three call-sites in the codebase each duplicated the same config-slice
+ list_authenticated_providers + post-processing pattern:

- hermes_cli/web_server.py /api/model/options
- tui_gateway/server.py model.options JSON-RPC
- tui_gateway/server.py model.save_key JSON-RPC

This consolidates them onto hermes_cli/inventory.py:

  load_picker_context() -> ConfigContext
      Replaces the 17-LOC config-slice (model.{default,name,provider,
      base_url}, providers:, custom_providers:) every consumer did
      inline.

  ConfigContext.with_overrides(*, current_provider=, current_model=,
                               current_base_url=) -> ConfigContext
      Truthy-only overlay for TUI agent-session state on top of disk
      config. Empty getattr(agent, ...) attrs MUST NOT clobber disk.

  build_models_payload(ctx, *, include_unconfigured, picker_hints,
                       canonical_order, max_models) -> dict
      Single payload builder. Delegates curation to
      list_authenticated_providers (does not call provider_model_ids
      per row \u2014 that pulls non-agentic models). picker_hints +
      canonical_order produce the TUI ModelPickerDialog shape;
      defaults match the dashboard's existing /api/model/options
      contract.

Two latent bugs fixed by consolidation:

1. The dashboard read cfg.get('custom_providers') directly, missing
   the v12+ keyed providers: form. Now both surfaces go through
   get_compatible_custom_providers().

2. The TUI's canonical-merge keyed on is_user_defined to decide order.
   Section 3 of list_authenticated_providers sets is_user_defined=True
   on rows from the providers: config dict even when the slug is
   canonical \u2014 that silently demoted them to the picker tail.
   _reorder_canonical now keys on slug membership instead.

Stats: +666 / -145 (net +521). Module 240 LOC; 18 behavior tests.

This PR replaces the rejected NousResearch#23369 (which bundled the consolidation
with new scriptable CLI surfaces \u2014 hermes models list/status, hermes
providers list \u2014 and a JSON contract that have no external user
demand). Just the refactor; the CLI surface is deferred to a separate
PR gated on actual demand.

Refs NousResearch#23359.
@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change comp/cli CLI entry point, hermes_cli/, setup wizard comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have labels May 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

@daimon-nous review

@daimon-nous

daimon-nous Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

Review: refactor(inventory) — extract shared ConfigContext + build_models_payload

Verdict: ✅ LGTM — clean, well-scoped refactor with a real bug fix included.

What it does

Extracts the duplicated "config-slice → list_authenticated_providers → post-process" pattern from 3 call-sites into a new hermes_cli/inventory.py module (240 LOC). The 3 consumers collapse to one-liners calling build_models_payload(load_picker_context(), ...).

Bug fix (real, confirmed on main)

All three pre-existing call-sites used cfg.get("custom_providers") which only reads the legacy list format. The new load_picker_context() routes through get_compatible_custom_providers(cfg) which also picks up v12+ keyed providers: entries. This is a genuine behavioral improvement for users who configure providers via the keyed schema.

Code quality observations

  1. ConfigContext.with_overrides() is well-designed — truthy-only semantics correctly handle the TUI's empty getattr(agent, "provider", "") when no agent is spawned yet. Immutable dataclass + replace() is clean.

  2. _apply_picker_hints heuristic is sound — distinguishes authenticated rows from skeleton rows via source == "canonical" and not models. This is correct because:

    • Section 2 of list_authenticated_providers emits source: "canonical" rows WITH populated models (they passed credential checks)
    • _append_unconfigured_rows emits source: "canonical" rows with EMPTY models (skeleton placeholders)

    Edge case handled: section 1 rows use source: "built-in" → correctly marked authenticated.

  3. _reorder_canonical keys on slug membership, not is_user_defined — correct design decision, well-documented in both code comments and a regression test. Prevents the canonical-slug-demotion bug.

  4. Test coverage is solid (18 tests, 378 LOC) — covers load_picker_context variants, with_overrides semantics, build_models_payload shape, picker_hints behavior, canonical_order regression, credential-leak canary, and frontend shape compatibility.

Minor observations (not blocking)

  • _append_unconfigured_rows uses .lower() for slug comparison in the seen set, while _reorder_canonical uses exact match. Functionally equivalent since all canonical slugs are lowercase, but slightly inconsistent. Not worth changing.

  • The load_config() vs _load_cfg() swap: the TUI gateway previously used its own _load_cfg() caching. Now it goes through hermes_cli.config.load_config() (also mtime-cached). Both do mtime-based staleness checks, so no performance regression. This is actually an improvement — one canonical config loader.

  • The PR correctly leaves 3 other call-sites unmigrated (cli.py interactive picker, hermes_cli/main.py aux-task switcher, gateway/run.py model switcher) for a follow-up. Good scope discipline.

CI status

3 checks fail (test, e2e, Windows footguns) — all reproduce identically on origin/main. Not introduced by this PR.

Summary

Well-scoped refactor that reduces duplication across 3 inventory call-sites, fixes a real v12+ provider visibility bug, and adds a regression guard for canonical-slug ordering. The consolidation ratio (145 deleted / 240 added = 0.60) is honest. Replaces the rejected #23369 which bundled unnecessary CLI surfaces. Ready to merge.

teknium1 added a commit that referenced this pull request May 14, 2026
The interactive CLI /model picker was the third call-site duplicating
the inline config-slice + list_authenticated_providers pattern that
PR #23666 consolidated for the dashboard and TUI. Route it through
load_picker_context() + build_models_payload() too so all surfaces
that show authenticated providers share one substrate.

Side effect: cli.py now also benefits from the latent v12+ keyed
providers fix (custom_providers populated via
get_compatible_custom_providers, not cfg.get raw).

The aux-task switcher (hermes_cli/main.py) and gateway model
switcher (gateway/run.py) deliberately stay on the legacy path —
they use different config sections (auxiliary.<task>.*) and a
different config loader (_load_gateway_config) respectively, so
forcing them through ConfigContext would either overload its
semantics or grow the module past the clean refactor scope.
@teknium1

Copy link
Copy Markdown
Contributor

Salvaged onto current main via #25450. Your inventory module and tests cherry-picked with authorship preserved; we also extended the consolidation to the CLI /model picker (cli.py) — fourth call-site on the legacy path that fits cleanly. The aux-task and gateway switchers stay on legacy because their config semantics differ. Thanks!

@teknium1 teknium1 closed this May 14, 2026
jsboige pushed a commit to jsboige/hermes-agent that referenced this pull request May 14, 2026
The interactive CLI /model picker was the third call-site duplicating
the inline config-slice + list_authenticated_providers pattern that
PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through
load_picker_context() + build_models_payload() too so all surfaces
that show authenticated providers share one substrate.

Side effect: cli.py now also benefits from the latent v12+ keyed
providers fix (custom_providers populated via
get_compatible_custom_providers, not cfg.get raw).

The aux-task switcher (hermes_cli/main.py) and gateway model
switcher (gateway/run.py) deliberately stay on the legacy path —
they use different config sections (auxiliary.<task>.*) and a
different config loader (_load_gateway_config) respectively, so
forcing them through ConfigContext would either overload its
semantics or grow the module past the clean refactor scope.
AlexFoxD pushed a commit to AlexFoxD/hermes-agent that referenced this pull request May 21, 2026
The interactive CLI /model picker was the third call-site duplicating
the inline config-slice + list_authenticated_providers pattern that
PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through
load_picker_context() + build_models_payload() too so all surfaces
that show authenticated providers share one substrate.

Side effect: cli.py now also benefits from the latent v12+ keyed
providers fix (custom_providers populated via
get_compatible_custom_providers, not cfg.get raw).

The aux-task switcher (hermes_cli/main.py) and gateway model
switcher (gateway/run.py) deliberately stay on the legacy path —
they use different config sections (auxiliary.<task>.*) and a
different config loader (_load_gateway_config) respectively, so
forcing them through ConfigContext would either overload its
semantics or grow the module past the clean refactor scope.
gweeteve pushed a commit to gweeteve/hermes-agent that referenced this pull request Jun 2, 2026
The interactive CLI /model picker was the third call-site duplicating
the inline config-slice + list_authenticated_providers pattern that
PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through
load_picker_context() + build_models_payload() too so all surfaces
that show authenticated providers share one substrate.

Side effect: cli.py now also benefits from the latent v12+ keyed
providers fix (custom_providers populated via
get_compatible_custom_providers, not cfg.get raw).

The aux-task switcher (hermes_cli/main.py) and gateway model
switcher (gateway/run.py) deliberately stay on the legacy path —
they use different config sections (auxiliary.<task>.*) and a
different config loader (_load_gateway_config) respectively, so
forcing them through ConfigContext would either overload its
semantics or grow the module past the clean refactor scope.
Seven74AI pushed a commit to Seven74AI/hermes-agent that referenced this pull request Jun 13, 2026
The interactive CLI /model picker was the third call-site duplicating
the inline config-slice + list_authenticated_providers pattern that
PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through
load_picker_context() + build_models_payload() too so all surfaces
that show authenticated providers share one substrate.

Side effect: cli.py now also benefits from the latent v12+ keyed
providers fix (custom_providers populated via
get_compatible_custom_providers, not cfg.get raw).

The aux-task switcher (hermes_cli/main.py) and gateway model
switcher (gateway/run.py) deliberately stay on the legacy path —
they use different config sections (auxiliary.<task>.*) and a
different config loader (_load_gateway_config) respectively, so
forcing them through ConfigContext would either overload its
semantics or grow the module past the clean refactor scope.
alt-glitch pushed a commit that referenced this pull request Jun 14, 2026
The interactive CLI /model picker was the third call-site duplicating
the inline config-slice + list_authenticated_providers pattern that
PR #23666 consolidated for the dashboard and TUI. Route it through
load_picker_context() + build_models_payload() too so all surfaces
that show authenticated providers share one substrate.

Side effect: cli.py now also benefits from the latent v12+ keyed
providers fix (custom_providers populated via
get_compatible_custom_providers, not cfg.get raw).

The aux-task switcher (hermes_cli/main.py) and gateway model
switcher (gateway/run.py) deliberately stay on the legacy path —
they use different config sections (auxiliary.<task>.*) and a
different config loader (_load_gateway_config) respectively, so
forcing them through ConfigContext would either overload its
semantics or grow the module past the clean refactor scope.
T02200059 pushed a commit to T02200059/hermes-agent that referenced this pull request Jun 18, 2026
The interactive CLI /model picker was the third call-site duplicating
the inline config-slice + list_authenticated_providers pattern that
PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through
load_picker_context() + build_models_payload() too so all surfaces
that show authenticated providers share one substrate.

Side effect: cli.py now also benefits from the latent v12+ keyed
providers fix (custom_providers populated via
get_compatible_custom_providers, not cfg.get raw).

The aux-task switcher (hermes_cli/main.py) and gateway model
switcher (gateway/run.py) deliberately stay on the legacy path —
they use different config sections (auxiliary.<task>.*) and a
different config loader (_load_gateway_config) respectively, so
forcing them through ConfigContext would either overload its
semantics or grow the module past the clean refactor scope.
liuchanchen pushed a commit to liuchanchen/hermes-agent that referenced this pull request Jul 3, 2026
The interactive CLI /model picker was the third call-site duplicating
the inline config-slice + list_authenticated_providers pattern that
PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through
load_picker_context() + build_models_payload() too so all surfaces
that show authenticated providers share one substrate.

Side effect: cli.py now also benefits from the latent v12+ keyed
providers fix (custom_providers populated via
get_compatible_custom_providers, not cfg.get raw).

The aux-task switcher (hermes_cli/main.py) and gateway model
switcher (gateway/run.py) deliberately stay on the legacy path —
they use different config sections (auxiliary.<task>.*) and a
different config loader (_load_gateway_config) respectively, so
forcing them through ConfigContext would either overload its
semantics or grow the module past the clean refactor scope.
liuchanchen pushed a commit to liuchanchen/hermes-agent that referenced this pull request Jul 3, 2026
The interactive CLI /model picker was the third call-site duplicating
the inline config-slice + list_authenticated_providers pattern that
PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through
load_picker_context() + build_models_payload() too so all surfaces
that show authenticated providers share one substrate.

Side effect: cli.py now also benefits from the latent v12+ keyed
providers fix (custom_providers populated via
get_compatible_custom_providers, not cfg.get raw).

The aux-task switcher (hermes_cli/main.py) and gateway model
switcher (gateway/run.py) deliberately stay on the legacy path —
they use different config sections (auxiliary.<task>.*) and a
different config loader (_load_gateway_config) respectively, so
forcing them through ConfigContext would either overload its
semantics or grow the module past the clean refactor scope.
kulikman pushed a commit to kulikman/hermes-agent that referenced this pull request Jul 16, 2026
The interactive CLI /model picker was the third call-site duplicating
the inline config-slice + list_authenticated_providers pattern that
PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through
load_picker_context() + build_models_payload() too so all surfaces
that show authenticated providers share one substrate.

Side effect: cli.py now also benefits from the latent v12+ keyed
providers fix (custom_providers populated via
get_compatible_custom_providers, not cfg.get raw).

The aux-task switcher (hermes_cli/main.py) and gateway model
switcher (gateway/run.py) deliberately stay on the legacy path —
they use different config sections (auxiliary.<task>.*) and a
different config loader (_load_gateway_config) respectively, so
forcing them through ConfigContext would either overload its
semantics or grow the module past the clean refactor scope.
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
The interactive CLI /model picker was the third call-site duplicating
the inline config-slice + list_authenticated_providers pattern that
PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through
load_picker_context() + build_models_payload() too so all surfaces
that show authenticated providers share one substrate.

Side effect: cli.py now also benefits from the latent v12+ keyed
providers fix (custom_providers populated via
get_compatible_custom_providers, not cfg.get raw).

The aux-task switcher (hermes_cli/main.py) and gateway model
switcher (gateway/run.py) deliberately stay on the legacy path —
they use different config sections (auxiliary.<task>.*) and a
different config loader (_load_gateway_config) respectively, so
forcing them through ConfigContext would either overload its
semantics or grow the module past the clean refactor scope.
waym0reom3ga added a commit to waym0reom3ga/autolycus-agent that referenced this pull request Jul 21, 2026
The interactive CLI /model picker was the third call-site duplicating
the inline config-slice + list_authenticated_providers pattern that
PR NousResearch#23666 consolidated for the dashboard and TUI. Route it through
load_picker_context() + build_models_payload() too so all surfaces
that show authenticated providers share one substrate.

Side effect: cli.py now also benefits from the latent v12+ keyed
providers fix (custom_providers populated via
get_compatible_custom_providers, not cfg.get raw).

The aux-task switcher (hermes_cli/main.py) and gateway model
switcher (gateway/run.py) deliberately stay on the legacy path —
they use different config sections (auxiliary.<task>.*) and a
different config loader (_load_gateway_config) respectively, so
forcing them through ConfigContext would either overload its
semantics or grow the module past the clean refactor scope.
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 comp/gateway Gateway runner, session dispatch, delivery comp/tui Terminal UI (ui-tui/ + tui_gateway/) P3 Low — cosmetic, nice to have type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants